stake.link

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

Vulnerability Details

As an example, the SDLPoolPrimary contract inherits SDLPool, and the SDLPool contract does not contain any storage gap. If in a future upgrade, an additional variable is added to the SDLPool contract, that new variable will overwrite the storage slot of the delegatorPool variable in the SDLPoolPrimary contract, causing unintended consequences.

https://github.com/Cyfrin/2023-12-stake-link/blob/main/contracts/core/sdlPool/SDLPoolPrimary.sol#L14

Similarly, the RewardsPoolController does not contain any storage gap either, and the SDLPool contract inherits RewardsPoolController. If a new variable is added to the RewardsPoolController contract in an upgrade, that variable will overwrite the name variable in SDLPool contract.

https://github.com/Cyfrin/2023-12-stake-link/blob/main/contracts/core/sdlPool/base/SDLPool.sol#L26

Recommendations

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

uint256[50] private __gap;