The RubiconMarket contract is meant to be upgradeable, but it inherits contracts that are not upgrade-safe. Adding new storage variables to inherited contracts will overwrite storage slots that have already been assigned on the proxy contract.
Proof of Concept
A Rubicon developer adds a new storage variables to the ExpiringMarket contract. It's an address field that holds the address of the owner who stopped the contract. That address field pushes all following storage slots down one, which pushes the initialized boolean of RubiconMarket to a slot and offset that now returns false. The Rubicon developer upgrades the RubiconMarket contract. Mallory inspects the storage variable for initialized and sees that it returns false, and so she calls the initialize method, which assigns her as the owner of the contract. Mallory now has admin privileges over critical contract functionality and can setMinSell, setBuyEnabled, setAqueductAddress, etc.
Tools Used
Manual analysis
Recommended Mitigation Steps
Follow best practices for upgradeable contracts and provide a storage gap so that new variables can be added safely. See OpenZeppelin's suggestions around different types of storage strategies for alternative solutions to this problem.
Lines of code
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L525-L530 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L551-L567 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L448-L450
Vulnerability details
Impact
The
RubiconMarket
contract is meant to be upgradeable, but it inherits contracts that are not upgrade-safe. Adding new storage variables to inherited contracts will overwrite storage slots that have already been assigned on the proxy contract.Proof of Concept
A Rubicon developer adds a new storage variables to the
ExpiringMarket
contract. It's anaddress
field that holds the address of the owner who stopped the contract. That address field pushes all following storage slots down one, which pushes theinitialized
boolean ofRubiconMarket
to a slot and offset that now returnsfalse
. The Rubicon developer upgrades theRubiconMarket
contract. Mallory inspects the storage variable forinitialized
and sees that it returns false, and so she calls theinitialize
method, which assigns her as theowner
of the contract. Mallory now has admin privileges over critical contract functionality and cansetMinSell
,setBuyEnabled
,setAqueductAddress
, etc.Tools Used
Manual analysis
Recommended Mitigation Steps
Follow best practices for upgradeable contracts and provide a storage gap so that new variables can be added safely. See OpenZeppelin's suggestions around different types of storage strategies for alternative solutions to this problem.
https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps