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

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L8 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L14-L15 https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L13-L14

Vulnerability details

Impact

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". 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.

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

Proof of Concept:

Several contracts are intended to be upgradeable contracts in the code base, including

• LRTConfig
• LRTDepositPool
• LRTOracle
• NodeDelegator

However, none of these contracts 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:

You can see here stating sponsor that the contracts are upgradeable, also the contest page on code4rena says that the contracts are upgradeable

As an example, there isn't any contract (in audit scope) in the repo not contain any storage gap. If in a future upgrade, an additional variable is added to any of the contract then it can potentially overwrite the beginning of the storage layout, causing critical misbehaviors in the system.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider defining an appropriate storage gap in each upgradeable parent contract at the end of all the storage variable definitions as follows:

uint256[50] __gap; // gap to reserve storage in the contract for future variable additions

Assessed type

Upgradable

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #49

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid