code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/proxy/UUPS.sol#L9 https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/token/ERC721.sol#L11 https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/utils/EIP712.sol#L9 https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/utils/Ownable.sol#L9 https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/utils/Pausable.sol#L7 https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/utils/ReentrancyGuard.sol#L6

Vulnerability details

Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

Impact

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.

See: https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps For a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

Proof of Concept

In the following context of the upgradeable contracts they are expected to use gaps for avoiding collision:

UUPS
ERC721
EIP712
Ownable
Pausable
ReentrancyGuard

However, none of these contracts contain storage gap. The storage gap is essential for upgradeable contract because "It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Refer to the bottom part of this article:

https://docs.openzeppelin.com/contracts/4.x/upgradeable

If a contract inheriting from a 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.

Github Permalinks

https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/proxy/UUPS.sol#L9 https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/token/ERC721.sol#L11 https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/utils/EIP712.sol#L9 https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/utils/Ownable.sol#L9 https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/utils/Pausable.sol#L7 https://github.com/code-423n4/2022-09-nouns-builder/blob/c682e196982ca6953fe9fcb85df02748795394fd/src/lib/utils/ReentrancyGuard.sol#L6

Tools Used

Manual analysis

Recommended Mitigation Steps

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;

GalloDaSballo commented 1 year ago

Dup of #358