MaslarovK.eth - No storage gap left for upgradeable contracts
Closed this issue · 1 comments
MaslarovK.eth
medium
No storage gap left for upgradeable contracts
Summary
No storage gap left in 'FlashRolloverLoan_G5' and 'LenderCommitmentGroup_Smart'.
Vulnerability Detail
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
Impact
Several contracts are intended to be upgradeable contracts in the code base, including
- FlashRolloverLoan_G5
- LenderCommitmentGroup_Smart
As an example, both of them are intended to act as the base contracts in the project. If the contract inheriting the base contract contains 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.
Code Snippet
Tool used
Manual Review
Recommendation
Add storage gap to the 'FlashRolloverLoan_G5' and 'LenderCommitmentGroup_Smart' contracts.
uint256[50] private __gap;Invalid based on sherlock rules. Additinally, storage collision is purely speculated.
Use of Storage gaps: Simple contracts with one of the parent contract not implementing storage gaps are considered low/informational.