search

hats-finance / Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d

Aleph Zero bridge to Ethereum
Apache License 2.0
0 stars 1 forks source link

No Storage Gap for Upgradeable Contract Might Lead to Storage Slot Collision #51

Open hats-bug-reporter[bot] opened 5 months ago

hats-bug-reporter[bot] commented 5 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x2fc8e2282cc7c28633a0a229cf8aa130832ed24b119d29ad5bcbc25073ece2a3 Severity: medium

Description: Description\ 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" (quote OpenZeppelin). 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, potentially causing loss of user fund or cause the contract to malfunction completely.

Refer to the bottom part of this article: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable

Attack Scenario\ https://github.com/hats-finance/Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d/blob/master/eth/contracts/Most.sol#L17

Alpha zero bridge is upgradeable however it doesn't contain storage gap. The storage gap is essential for upgradeable contract because "It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Refer to the bottom part of this article:

https://docs.openzeppelin.com/contracts/3.x/upgradeable

Recommendation\ 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;

NOTE: This issue has been marked as Med many times before, you can see it on solodit.xyz

PlamenTSV commented 5 months ago

This is required only for contracts that get inherited by other contracts, since you would get a collision on upgrading the parent. Most.sol is a base contract, so adding more variables via upgrading would not lead to any issues, since there is no other contract to have reserved that storage. Could be a minor depending on dev team intentions, but in the current context it's not med

krzysztofziobro commented 4 months ago

Exactly, it isn't meant to be used as base in inheritance - doesn't apply