No storage gap for upgradeable contract might lead to storage slot collisions

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L12 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L19 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L19 https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L18 https://github.com/code-423n4/2023-11-kelp/blob/main/src/RSETH.sol#L14

Vulnerability details

Impact

For upgradeable contracts, it is imperative to incorporate a storage gap, as articulated by OpenZeppelin: "allowing developers to seamlessly introduce new state variables in the future without compromising storage compatibility with existing deployments." The absence of a storage gap could pose significant challenges when crafting new implementation code. Without this safeguard, the introduction of new variables in the base contract might overwrite variables in child contracts during an upgrade. Such a scenario could lead to unintended and severe consequences, potentially resulting in the loss of user funds or complete contract malfunction.

Refer to the bottom part of this article: OpenZeppelin Upgrades Plugins Documentation

Proof of Concept

Several contracts in the codebase, including LRTConfig, LRTDepositPool, LRTOracle, NodeDelegator, and RSETH, are intended to be upgradeable contracts. Regrettably, none of these contracts currently implement a storage gap. The absence of a storage gap is a critical deficiency in upgradeable contracts because, as OpenZeppelin emphasizes, it enables the addition of new state variables without compromising compatibility with existing deployments. For detailed guidance, please refer to the relevant section in the OpenZeppelin documentation: OpenZeppelin Upgradeable Contracts Documentation

As an illustrative example, both LRTConfig and LRTDepositPool are intended to serve as base contracts in the project. If a contract inheriting the base contract introduces additional variables, the base contract becomes unable to incorporate any new variables during an upgrade, as this action would overwrite variables declared in the child contract. This limitation significantly curtails the upgradeability of contracts.

Tools Used

Manual Review

Recommended Mitigation Steps

It is strongly recommended to address this issue by incorporating an appropriate storage gap at the end of upgradeable contracts, as demonstrated in the code snippet below. For implementation, please refer to OpenZeppelin's upgradeable contract templates.

uint256[50] private __gap;

Assessed type

Upgradable

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[raymondfam]

L-10 from the bot.

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid