Upgradeable contracts should implement ERC-7201 name spacing to reduce the risk of storage collisions

Summary

According to the sponsor, the following contracts in scope are upgradeable : GladiusReactor && RubiconFeeController. DSAuthand ProxyConstructorare inherited contracts in RubiconFeeController. These two inherited contracts should implement ERC-7201 or similar to reduce the risk of storage collision. On top of this, BaseGladiusReactor, which is inherited by GladiusReactor, uses ReentrancyGuard by Openzeppelin. For the same reason, it should instead use ReentrancyGuardUpgradeable (https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/utils/ReentrancyGuardUpgradeable.sol)

Vulnerability Detail

When upgrading a contract, there is the risk of storage collision if the storage layout is not compatible between different versions of the logic contract.

Impact

When a contract is upgraded, the slot to be used for storing a given variable can be the same slot previously used by a different variable. This would cause the contract to malfunction potentially putting the contract at risk.

Code Snippet

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol#L16-L19 https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/GladiusReactor.sol#L18

Tool used

Manual Review

Recommendation

Implementation of ERC-7201 or any other method for reducing the risk of storage collisions.

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

0xRstStn - Upgradeable contracts should implement ERC-7201 name spacing to reduce the risk of storage collisions #72