search

code-423n4 / 2022-02-skale-findings

0 stars 0 forks source link

Not leaving storage gaps for future upgrade #44

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L530 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBox.sol#L120

Vulnerability details

Impact

These contracts use OpenZepplin's transparent upgradeable pattern to manage the upgradeability of the system. When working with multi-level inheritance, by adding a new variable in the parent contract may potentially overwrite the beginning of a child's storage layout, thus messing up the entire storage.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

For example MessageProxyForSchain inherits MessageProxy. https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/MessageProxyForSchain.sol#L60

MessageProxy doesn't have a gap in the storage at the end of the contract. https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L529

By adding a new storage variable in MessageProxy, it may take the storage slot of keyStorage.

Another base layer contract is DepositBox https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBox.sol#L120

and Twin https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/Twin.sol#L106

Tools Used

Recommended Mitigation Steps

Leave a gap at the end of the parent contract such as

    uint256[49] private __gap;
cstrangedk commented 2 years ago

Good recommendation, however no vulnerability is apparent. Suggest this is more of a QA report in relation to code quality for supporting upgradeability.

GalloDaSballo commented 2 years ago

I agree with both sides in saying that it is prudent to add a storage gap for future upgrades. However there's no vulnerability here, for that reason I believe QA to be more appropriate.