GIP-31: Should Gnosis Chain perform a Hardfork to upgrade the token contract vulnerable to the reentrancy attack?
- Let’s do this!
- Make no changes
0 voters
GIP: 31
title: Should Gnosis Chain perform a Hardfork to upgrade the token contract vulnerable to the reentrancy attack?
author: GnosisDAO
status: Draft
type: Meta
created: 2022/03/17
On 15th March 2022 tokens bridged with the OmniBridge were used in attacks on the Agave and Hundred Finance protocols on the Gnosis Chain.
Technical Background
The Omnibridge inherited functionality from the original version of the TokenBridge, using the ERC677 extension for the token contract to simplify the UX. ERC677-compatible tokens are bridged using the transferAndCall invocation rather than a sequence of approve and transferFrom calls.
This behavior was extended to the transfer method to accommodate users accidentally calling transfer rather than transferAndCall when bridging tokens. When called, tokens are bridged instead of locked in the bridge contract.
The behavior to execute a call after a transfer opened the door to potential reentrancy attacks.
An audit on 9 November, 2020 revealed an incompatibility with the ERC20 standard, and the callAfterTransfer method was modified to fix the issue.
However, this new implementation could not be used for previously bridged tokens since the token contracts are not upgradable. Although a proxy template was used by the bridged tokens to reduce gas usage for the tx executing the very first token transfer, the upgradability functionality was not implemented for security reasons.
Prior to the new implementation, 517 tokens had already been bridged to the Gnosis Chain including WETH, USDC, GNO, LINK, WBTC, and many others.
Both protocols, Agave and Hundred Finance, forks of AAVE and Compound used a forked codebase prone to reentrancy attacks enabled on token transfers.
Possible Solutions
For the 517 tokens - as the tokens are not upgradable by design - there are three options:
-
Do nothing - but make everyone very aware that the tokens have this callback functionality. This will likely mean that many contracts that are safe to use on mainnet are not necessarily safe to use on GC in case they are prone to reentrancy attacks on token transfers. We don’t see this as a viable solution as we want developers to easily port applications to Gnosis Chain without modifying code.
-
Promoting an optional migration to tokens without this functionality. E.g. there could be a wrapper token of all tokens affected without the callback. DeFi protocols would be advised to use the wrapped tokens if they are not sure that their contracts are reentrancy safe. We don’t see this solution as optimal as it requires additional wrapping of tokens disrupting the user experience. Some tokens, like GNO, are locked for a long time and won’t be converted anytime soon.
-
A hardfork of the chain that could replace the bytecode of those tokens in a way that removes the callback. A primary concern would be that this hard fork would not affect anyone negatively potentially relying on this extra functionality. We did investigate this issue and found only one project (HOPR) was affected by this issue.
Based on this analysis we advocate for a hardfork of Gnosis Chain as it allows the least disruptive experience for users and developers.
Hardfork Specification
The hard fork assumes to update Gnosis Chain spec file (explicitly or through node upgrading with the new internal spec) on all nodes and upgrade the nodes themselves to the new version supporting dao-like hard forks.
Majority of the nodes must be upgraded before the hard fork block defined in the spec.
The spec file would contain the new instruction in engine.authorityRound.params section. Example:
rewriteBytecode: {
"21300000": {
"0x1234000000000000000000000000000000000001": "0xbytecode1...",
"0x1234000000000000000000000000000000000002": "0xbytecode2..."
}
}
It defines the block number at which the bytecodes should be rewritten for the specified contracts (can be more than one per block).
The rewriteBytecode option can have multiple blocks (for possible future similar hard forks), e.g.:
rewriteBytecode: {
"21300000": {
"0x1234000000000000000000000000000000000001": "0xbytecode1...",
"0x1234000000000000000000000000000000000002": "0xbytecode2..."
},
"22200000": {
"0x1234000000000000000000000000000000000003": "0xbytecode3...",
"0x1234000000000000000000000000000000000004": "0xbytecode4..."
}
}