Platform: Code4rena
Start Date: 23/05/2022
Pot Size: $50,000 USDC
Total HM: 44
Participants: 99
Period: 5 days
Judge: hickuphh3
Total Solo HM: 11
Id: 129
League: ETH
Rank: 27/99
Findings: 2
Award: $453.64
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: 0x1337
Also found by: broccolirob
401.6035 USDC - $401.60
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L448-L449 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L525-L535
For upgradeable contracts, there must be storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Otherwise it may be very difficult to write new implementation code. Without storage gap, the variable in child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences to the child contracts.
Refer to the bottom part of this article: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable
As an example, the ExpiringMarket contract inherits SimpleMarket, and the SimpleMarket contract does not contain any storage gap. If in a future upgrade, an additional variable is added to the SimpleMarket contract, that new variable will overwrite the storage slot of the stopped variable in the ExpiringMarket contract, causing unintended consequences.
Similarly, the ExpiringMarket does not contain any storage gap either, and the RubiconMarket contract inherits ExpiringMarket. If a new variable is added to the ExpiringMarket contract in an upgrade, that variable will overwrite the buyEnabled variable in ExpiringMarket contract.
Manual review
Recommend adding appropriate storage gap at the end of upgradeable contracts such as the below. Please reference OpenZeppelin upgradeable contract templates.
uint256[50] private __gap;
#0 - HickupHH3
2022-06-15T13:37:42Z
out of curiosity, is there a reason why the openzeppelin-upgrades package isn't used?
according to the in the README, Transparent Upgradeable Proxies is used to make sure "all contracts can be iterated and improved over time." seems like the initializable contract should've been inherited and utilized so that you wouldnt have to worry about adding in storage gaps.
#1 - bghughes
2022-07-06T20:55:19Z
out of curiosity, is there a reason why the openzeppelin-upgrades package isn't used?
according to the in the README, Transparent Upgradeable Proxies is used to make sure "all contracts can be iterated and improved over time." seems like the
initializablecontract should've been inherited and utilized so that you wouldnt have to worry about adding in storage gaps.
Optimism had a custom compiler that required custom proxies. This led to the package not working otherwise I would have used it.
It seems like always extending the top-level storage contract as a practice should avoid any issues here, right? It is a good issue that highlights I should never try to extend inherited contract's storage
#2 - HickupHH3
2022-07-07T02:43:47Z
Ahhh I see. It depends on future upgrades that will affect the storage layout. Inheriting from Ownable and Pausable for instance shouldnt affect upgradeability much because I dont think their required functionality will drastically change.
But yeah, ensuring there is sufficient gap is to future-proof the contracts. Worst case, do a new deployment.
#3 - bghughes
2022-07-11T22:56:05Z
Ahhh I see. It depends on future upgrades that will affect the storage layout. Inheriting from Ownable and Pausable for instance shouldnt affect upgradeability much because I dont think their required functionality will drastically change.
But yeah, ensuring there is sufficient gap is to future-proof the contracts. Worst case, do a new deployment.
I just ran into this attempting to add the Reentrancy Gaurd to a contract. To be clear warden should I add this to the end of the existing base contract? For example, appending the uint256[50] gap to the base contract before inheritance?
Thanks and good issue
#4 - bghughes
2022-07-11T22:59:00Z
I'd love to know the appropriate way to bolt on something like ReentrancyGuard onto an existing proxy-wrapped contract - is it even possible?
My game plan is to bring the nonReentrant modifier into the top-level contract to only extend storage. Thank you again warden!
#5 - HickupHH3
2022-07-12T08:26:00Z
🌟 Selected for report: IllIllI
Also found by: 0x1337, 0x1f8b, 0x4non, 0xDjango, 0xKitsune, 0xNazgul, 0xf15ers, ACai, AlleyCat, Bahurum, BouSalman, CertoraInc, Chom, Dravee, ElKu, FSchmoede, Funen, GimelSec, Hawkeye, JC, JMukesh, Kaiziron, MaratCerby, Metatron, PP1004, Picodes, Ruhum, SmartSek, StErMi, TerrierLover, UVvirus, UnusualTurtle, WatchPug, Waze, _Adam, asutorufos, berndartmueller, blackscale, blockdev, broccolirob, c3phas, catchup, cryptphi, csanuragjain, defsec, delfin454000, dipp, eccentricexit, ellahi, fatherOfBlocks, gzeon, hansfriese, horsefacts, hubble, ilan, joestakey, kebabsec, minhquanym, oyc_109, parashar, pauliax, rotcivegaf, sach1r0, sashik_eth, shenwilly, simon135, sorrynotsorry, sseefried, throttle, unforgiven, xiaoming90
52.0351 USDC - $52.04
Defining initial values for variables when declaring them in a contract like in the code below does not work for upgradeable contracts.
Refer to explanation below:
Also, one should not leave the implementation contract uninitialized. None of the implementation contracts in the code base contains the code recommended by OpenZeppelin below, or an empty constructor with the initializer modifier.
/// @custom:oz-upgrades-unsafe-allow constructor constructor() { _disableInitializers(); }
Refer to the link below:
#0 - HickupHH3
2022-06-25T02:07:22Z
true in theory, but initialize() re-sets the mentioned variables to true. non-crit