No Storage Gap for Upgradable Contracts

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol#L10
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L11

Vulnerability details

Impact

The ERC20Upgradable.sol and ERC4626Upgradable.sol contracts are abstract, upgradable contracts where inheriting may introduce new storage variables. In order to safely upgrade the contracts which may include adding new storage variables, a storage gap should be added to both abstract contracts. The OpenZeppelin TimelockControllerUpgradable.sol contract demonstrates how this can be done.

If no storage gap is added and if the upgradable contracts introduce new variables, this may cause storage collisions which will result in the overriding of existing variables in the inheriting contract.

Proof of Concept

• https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol
• https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol

Recommended Mitigation Steps

It's recommended that the upgradable contracts ERC20Upgradable.sol and ERC4626Upgradable.sol include a storage gap similar to the following either at the foot or the head of the contract as a storage variable:

abstract contract ERC20Upgradeable is Initializable {

============================ SNIP ============================

	mapping(address => uint256) public balanceOf;

	mapping(address => mapping(address => uint256)) public allowance;

	/*//////////////////////////////////////////////////////////////
                            EIP-2612 STORAGE
    //////////////////////////////////////////////////////////////*/

	uint256 internal INITIAL_CHAIN_ID;

	bytes32 internal INITIAL_DOMAIN_SEPARATOR;

	mapping(address => uint256) public nonces;

	uint256[48] private __storageGap;

============================ SNIP ============================

	

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Better because of suggested remediation

[emersoncloud]

This is a fair point, but only an issue if we intend to modify ERC20Upgradeable or ERC4626Upgradeable implementation contract behind a proxy.

[GalloDaSballo]

After consulting the Rulebook

Am downgrading to Low Severity.

The Sponsor may not extend storage of child contracts, that is not inherently a vulnerability but a risk we're flagging as QA - Low Severity

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-c