No Storage Gap for Upgradeable Contracts

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/main/contracts/BlurExchange.sol#L30

Vulnerability details

Impact

For upgradeable contracts, inheriting contracts may introduce new variables. In order to be able to add new variables to the upgradeable contract without causing storage collisions, a storage gap should be added to the upgradeable acontract.

If no storage gap is added, when the upgradable contract introduces new variables,
it may override the variables in the inheriting contract.

Storage gaps are a convention for reserving storage slots in a base contract, allowing future versions of that contract to use up those slots without affecting the storage layout of child contracts.
To create a storage gap, declare a fixed-size array in the base contract with an initial number of slots.
This can be an array of uint256 so that each element reserves a 32 byte slot. Use the naming convention __gap so that OpenZeppelin Upgrades will recognize the gap:

Classification for a similar problem:
https://code4rena.com/reports/2022-05-alchemix/#m-05-no-storage-gap-for-upgradeable-contract-might-lead-to-storage-slot-collision

contract Base {
    uint256 base1;
    uint256[49] __gap;
}

contract Child is Base {
    uint256 child;
}

Openzeppelin Storage Gaps notification:

Storage Gaps
This makes the storage layouts incompatible, as explained in Writing Upgradeable Contracts. 
The size of the __gap array is calculated so that the amount of storage used by a contract 
always adds up to the same number (in this case 50 storage slots).

Proof of Concept

BlurExchange contract is intended to be upgradeable, including the inherited contracts ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable

However, neither ReentrancyGuarded nor the EIP712 or OwnableUpgradeable  contract contains storage gaps.

The BlurExchange contract contains storage variables, therefore the base contracts ReentrancyGuarded and EIP712 or OwnableUpgradeable cannot be upgraded to include any additional variables because it would overwrite the variables declared in the child contract BlurExchange. This greatly limits contract upgradeability.

Recommended Mitigation Steps

Consider adding a storage gap at the end of the upgradeable abstract contract

uint256[50] private __gap;

[blur-io-toad]

Helpful tweak, but not a vulnerability. I think this should be QA

[GalloDaSballo]

We're having discussions about standardizing the severity for this type of findings:
code-423n4/org#55

With the sponsor confirming and resolving, considering that library contract have no gaps.

Will mark the finding QA - Low Severity.

I recommend the sponsor to not add new storage slots to Library Contracts as they are not set to be changed in that way (and most projects never do, see discussion linked).

L