xAriextz - Founders won't get the ownership percentage they deserve when reservedUntilTokenId >= 100

[sherlock-admin2]

xAriextz

high

Founders won't get the ownership percentage they deserve when reservedUntilTokenId >= 100

Summary

For adding funders of a token the function _addFounders() is used. This function will set which tokens each founder will receive depending in their founderPct by storing the founder's wallet in the tokenRecipient.

                // Used to store the base token id the founder will recieve
                uint256 baseTokenId = reservedUntilTokenId;


                // For each token to vest:
                for (uint256 j; j < founderPct; ++j) {
                    // Get the available token id
                    baseTokenId = _getNextTokenId(baseTokenId);


                    // Store the founder as the recipient
                    tokenRecipient[baseTokenId] = newFounder;


                    emit MintScheduled(baseTokenId, founderId, newFounder);


                    // Update the base token id
                    baseTokenId = (baseTokenId + schedule) % 100;
                }
            }

The baseTokenId is selected by using the function _getNextTokenId().

    function _getNextTokenId(uint256 _tokenId) internal view returns (uint256) {
        unchecked {
            while (tokenRecipient[_tokenId].wallet != address(0)) {
                _tokenId = (++_tokenId) % 100;
            }


            return _tokenId;
        }
    }

Later, when the tokens are minted, they will be minted to a founder depending in the tokenId to be minted and the tokenRecipient that is fulfilled in _addFounders() as shown before.

    function _isForFounder(uint256 _tokenId) private returns (bool) {
        // Get the base token id
        uint256 baseTokenId = _tokenId % 100;


        // If there is no scheduled recipient:
        if (tokenRecipient[baseTokenId].wallet == address(0)) {
            return false;


            // Else if the founder is still vesting:
        } else if (block.timestamp < tokenRecipient[baseTokenId].vestExpiry) {
            // Mint the token to the founder
            _mint(tokenRecipient[baseTokenId].wallet, _tokenId);


            return true;


            // Else the founder has finished vesting:
        } else {
            // Remove them from future lookups
            delete tokenRecipient[baseTokenId];


            return false;
        }
    }

As we can see in _isForFounder(), all the possible values for tokenRecipient should be between 0-99, since the baseTokenId < 100. The problem is that the first baseTokenId used for storing the founders information in tokenRecipient in _addFounders() will be wrongly computed whenever the reservedUntilTokenId >= 100.

Vulnerability Detail

Note that this vulnerability doesn't arise in the first deployment since reservedUntilTokenId is not yet assigned in the initialize before calling _addFounders() and we have variable shadowing. However, the issue will arise when the founders are updated by calling updateFounders().

Here is an example where the founder won't get any tokens allocated due to the miscalculation of the first baseTokenId when reservedUntilTokenId >= 100, let's say that reservedUntilTokenId == 200 just to have an example.

1. There is a token with some founders.
2. Founders are updated.
   2.1. All founders info is cleaned.
   2.2. The new founder is Alice, who has a founderPct == 1, meaning that for every 100 tokens minted she should receive 1.
   2.3. _addFounders() is called with Alice's info.
   2.3.1. As we can see, baseTokenId = reservedUntilTokenId, so the function _getNextTokenId() will be called with 200 as a parameter. The function will return 200 since the while won't be entered in the first iteration.
   2.3.2. tokenRecipient[200] = aliceFounderInfo will be set. As she only has founderPct == 1, the function _addFounders() ends.
3. Whenever a new token is minted, the function _isForFounder() will check if the token should be minted to Alice, but she won't receive any tokens since the baseTokenId < 100 always in that function, and her assigned slot is tokenRecipient[200].

Impact

Each time that _addFounders() is called in a Token where reservedUntilTokenId >= 100 the first slot assigned in the tokenRecipient will be wrongly set, meaning that the first founder to be added will receive less Tokens than expected. In total, founders will receive 1% less tokens than expected. In cases where the founders are supposed to receive a small percentage of the NFTs the impact will be bigger. If the founders are expected to receive 1% of the tokens, they won't receive any tokens.

Code Snippet

https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L120-L194

Tool used

Manual Review

Recommendation

Fix the _getNextTokenId() function so that the tokenId returned is always between 0-99 as expected. A possible fix would be changing the while for a do while.

Duplicate of #42