sherlock-audit / 2024-02-rubicon-finance-judging

5 stars 3 forks source link

valentin2304 - No Storage Gap for Upgradeable Contract Might Lead to Storage Slot Collision #42

Closed sherlock-admin closed 9 months ago

sherlock-admin commented 9 months ago

valentin2304

medium

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

Summary

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

Vulnerability Detail

BaseGladiusReactor contract is intended to be an upgradeable contract in the code base

However, BaseGladiusReactor does not contain a storage gap. The storage gap is essential for upgradeable contracts 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

As an example, BaseGladiusReactor is intended to act as the base contract in the project. If the contract inheriting the base contract contains an additional variable, then the base contract cannot be upgraded to include any additional variable, because it would overwrite the variable declared in its child contract. This greatly limits contract upgradeability.

Impact

Medium

Code Snippet

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L19

Tool used

Manual Review

Recommendation

Recommend adding appropriate storage gap at the end of upgradeable contracts such as the one below. Please reference OpenZeppelin upgradeable contract templates.

uint256[50] private __gap;
sherlock-admin commented 9 months ago

3 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

Low. "Storage Slot Collision issue" is Low according to the Sherlock documentation

PNS commented:

Simple contracts with one of the parent contract not implementing storage gaps are considered low/informational.

0xAadi commented:

Invalid: OOS