Krace - Incorrect implementation of _getNextTokenId causing the loss of the founder's token

[sherlock-admin2]

Krace

medium

Incorrect implementation of _getNextTokenId causing the loss of the founder's token

Summary

_getNextTokenId is used to find the next available baseTokenId in the range from 0 to 99 for a founder. However, if reservedUntilTokenId surpasses 99, getNextTokenId will initially return a number greater than 99. This leads to tokenRecipient being unable to store the recipient correctly, causing the founder to lose tokens.

Vulnerability Detail

The _addFounders function stores the newFounder for baseTokenId in the mapping tokenRecipient. The baseTokenId is determined by the _getNextTokenId function, and the initial value of baseTokenId passed into _getNextTokenId is reservedUntilTokenId.

                // 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;
                }

Typically, _getNextTokenId returns a _tokenId within the range of 0 to 99. However, if reservedUntilTokenId exceeds 99, the while loop won't run because tokenRecipient[reservedUntilTokenId].wallet is equal to zero. As a result, _getNextTokenId will directly return the value of reservedUntilTokenId, which is a number greater than 99.

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

            return _tokenId;
        }
    }

Consider the usage of tokenRecipient in the _isForFounder function. The baseTokenId is determined by _tokenId % 100, implying that tokenRecipient[reservedUntilTokenId] will never be utilized. Consequently, the associated founder won't receive their token.

    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;
        }
    }

Impact

Founder cannot obtain the token that he was originally supposed to receive, facing economic loss.

Code Snippet

https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/db232c649b425c36f5a93607c95cfdf0e5962b2f/nouns-protocol/src/token/Token.sol#L166-L169
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/db232c649b425c36f5a93607c95cfdf0e5962b2f/nouns-protocol/src/token/Token.sol#L186-L194
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/db232c649b425c36f5a93607c95cfdf0e5962b2f/nouns-protocol/src/token/Token.sol#L263

Tool used

Manual Review

Recommendation

Maybe you can mod _tokenId by 100 before return.

diff --git a/nouns-protocol/src/token/Token.sol b/nouns-protocol/src/token/Token.sol
index 154ecf8..9e80442 100644
--- a/nouns-protocol/src/token/Token.sol
+++ b/nouns-protocol/src/token/Token.sol
@@ -189,7 +189,7 @@ contract Token is IToken, VersionedContract, UUPS, Ownable, ReentrancyGuard, ERC
                 _tokenId = (++_tokenId) % 100;
             }

-            return _tokenId;
+            return _tokenId % 100;
         }
     }

Duplicate of #42