Art Gobblers contest - RaymondFam's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/ArtGobblers.sol#L458 https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/utils/token/GobblersERC721.sol#L166

Vulnerability details

Impact

The mapping of user addresses to their account data could have gobblersOwned doubly incremented, messing up the account tally in the mapping below that is inherited by ArtGobblers.sol:

https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/utils/token/GobblersERC721.sol#L44

This could affect or disrupt all future function calls associated with it.

Proof of Concept

Whenever a Dutch auction were to be concluded for a legendary gobbler, the comment on line 457 of ArtGobblers.sol would say 1 being added to getUserData[msg.sender].gobblersOwned to factor in the new legendary. However, when the inherited _mint(msg.sender, gobblerId) was called on line 469, getUserData[to].gobblersOwned would end up incremented again in line 166 of GobblersERC721.sol.

Recommended Mitigation Steps

Refactor line 458 of ArtGobblers.sol as follows:

            getUserData[msg.sender].gobblersOwned = uint32(getUserData[msg.sender].gobblersOwned - cost);

or, remove lines 165 - 167 of GobblersERC721.sol.

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/ArtGobblers.sol#L311-L321 https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/Pages.sol#L177-L183 https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/utils/rand/ChainlinkV1RandProvider.sol#L55-L58 https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/Goo.sol#L83-L84 https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/utils/GobblerReserve.sol#L24

Vulnerability details

Impact

Zero address/value and/or empty string/bytes checks should be implemented at the constructor. In the event a mistake was made, not only that all calls associated with this incident would be non-functional, the contract would also need to be redeployed if no setter functions were catered for these affected state variables.

Proof of Concept

As an example, one or more of the following scenarios associated with ArtGobblers.sol could transpire, rendering the deployed contract obsolete:

1. A zero address could accidentally be assigned to goo, pages, team, community or randProvider.
2. An empty string might be assigned to BASE_URI or UNREVEALED_URI.
3. A zero value might be assigned to minStart.
4. An empty byte might also be assigned to merkleRoot.

Recommended Mitigation Steps

As an example, the following lines of codes should be inserted to the constructor body of ArtGobblers.sol:

if (_mintStart == 0) revert ZeroValue();
if (_merkleRoot == "") revert EmptyByte();
if (_goo == address(0) || _pages == address(0) || _team == address(0) || _community == address(0)
    _randProvider == address(0)) revert AddressZero();
if (BASE_URI == "" || UNREVEALED_URI == "") revert EmptyString();

Note: The custom errors will have to be declared above the constructor.