search

hats-finance / Intuition-0x538dbadc50cc87b281cd655f1edbc6ebda02a66a

The smart contracts of the Intuition protocol v1.
https://intuition.systems
Other
0 stars 1 forks source link

No storage gap for the upgradable contracts #60

Open hats-bug-reporter[bot] opened 4 days ago

hats-bug-reporter[bot] commented 4 days ago

Github username: @dinkras Twitter username: dinkras Submission hash (on-chain): 0xf858b671d16d9d90799812b4759408125cf81fb5309da6ccc29460e5a3246bd2 Severity: medium

Description: Description\ AtomWallet.sol and EthMultiWallet.sol are intended to be upgraded in the future via the Proxy pattern. However, no storage gap is present inside those contracts.

For upgradeable contracts, there must be a 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 and storage collisions may occur.

Without a storage gap, the variable in the 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 for the child contracts.

Attack Scenario\ Scenario A 1) The protocol team has released a new version of EthMultiVault -> EthMultiVaultV2 as shown as an example from the team here: https://github.com/hats-finance/Intuition-0x538dbadc50cc87b281cd655f1edbc6ebda02a66a/blob/main/test/EthMultiVaultV2.sol#L8

  1. At some point the team has found a vulnerability in the base contract and releases a fix of EthMultiVault which includes new/updated storage variables
  2. The next deployments of EthMultiVaultV2 will have storage collisions with the latest version of EthMultiVault due to the missing storage gap

Scenario B 1) The protocol team decides AtomWallet.sol or EthMultiWallet.sol to inherit a new contract B which has a new storage variable 2) Contract B storage variable overrides the first storage slot variables of those contracts

  1. Dev team deploys the new versions of the contracts
  2. Storage collision occurs

Attachments

  1. Proof of Concept (PoC) File Same as the content inside the Attack Scenario section

  2. Revised Code File (Optional)

Recommendation: Introduce storage gap/s insde AtomWallet.sol and EthMultiWallet.sol as recommended from the OpenZeppelin docs (Storage Gaps section)

uint256[gap_size] private __gap;

mihailo-maksa commented 4 days ago

Our Comment on the Issue: The reported issue concerning the absence of a storage gap in AtomWallet.sol and EthMultiVault.sol has been reviewed. Here is our comprehensive perspective:

Storage Gap Importance: For upgradeable contracts using the proxy pattern, it can be beneficial to include storage gaps. This allows developers to add new state variables in the future without compromising storage compatibility. Without a storage gap, storage collisions may occur, potentially causing unintended and serious consequences for child contracts.

Current Implementation: The current implementation of AtomWallet.sol and EthMultiVault.sol does not include a storage gap. This means that future upgrades or the addition of new base contracts with additional storage variables could lead to storage collisions if additional precaution is not taken.

Potential Risks:

  1. Storage Collisions: If new variables are added in future upgrades without a storage gap, they might overwrite existing variables, leading to unpredictable behavior and security vulnerabilities.
  2. Future Upgrades: The absence of a storage gap makes it difficult to maintain compatibility with future contract versions, limiting the ability to enhance and expand contract functionality safely.

Enhancement Suggestion: Introducing a storage gap in AtomWallet.sol and EthMultiVault.sol is a prudent enhancement. This will provide a buffer for adding new state variables in future upgrades, ensuring that storage layouts remain compatible and preventing collisions.

Severity Assessment: Since this issue does not introduce immediate vulnerabilities but poses significant risks for future upgrades, it is classified as a medium severity enhancement. Addressing this issue will ensure the contracts remain robust and secure in the long term.

Conclusion: Implementing a storage gap in AtomWallet.sol and EthMultiVault.sol is a valuable enhancement to prevent storage collisions and maintain compatibility with future upgrades. This will align the contracts with best practices for upgradeable contracts and ensure their long-term reliability.

Status: This issue is a valid enhancement.

Suggested Fix: Suggested fix makes sense and can potentially be accepted.

Comment for the Reporter: Thank you for highlighting this important issue. Introducing storage gaps in AtomWallet.sol and EthMultiVault.sol is a potentially valuable enhancement to prevent storage collisions in future upgrades. This is considered as an enhancement, rather than as a security vulnerability. We will consider implementing this to ensure compatibility with future upgrades.

dinkras commented 3 days ago

@mihailo-maksa Even though the issue does not possess immediate vulnerabilities, the risk of a storage collision in the future upgrades is high. I disagree that the issue should be classified as an enhancement and not a vulnerability due to the high risk of storage collisions in the future upgrades. In previous audits of different protocols, even OpenZeppelin gave the same issue a Medium severity

The same issue reported on different platforms and protocols OpenZeppelin report (Medium) Code4rena report of alchemix protocol (Medium) Code4rena report of Rubicon protocol (Medium) HalbornSecurity report (Medium) Another instance of the same vulnerability (Medium)