rvierdiiev - In case if reservedUntilTokenId > 99, then owner will never get token vested

[sherlock-admin2]

rvierdiiev

high

In case if reservedUntilTokenId > 99, then owner will never get token vested

Summary

In case if reservedUntilTokenId > 99, then _getNextTokenId function will return early without doing modulo operation and as result tokenRecipient will be set with value bigger than 99 for owner, which means that this tokens will not be never vested.

Vulnerability Detail

In case if dao decided to move from l1 to l2, then they will deploy new dao on l2 using L2MigrationDeployer and that they will provide reservedUntilTokenId param to the Token contract as the amount of already minted tokens on l1, to not allow minting with same ids.

During Token initialization _addFounders function will be called, which will set vesting positions of owners to the tokenRecipient mapping.

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

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

Here, we need to note that baseTokenId == reservedUntilTokenId and we need to look how next baseTokenId is calculated in the _getNextTokenId function.

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

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

The purpose of this function is to check if tokenRecipient[_tokenId] is empty. In case if it is, then this means that this position doesn't belong to anyone else and can be used, otherwise _tokenId is incremented and moduled as we work with positions 0-99 only.

The problem is that in case if reservedUntilTokenId is bigger than 99, which should be the case almost 100% of migrations, then _tokenId inside _getNextTokenId will not be increased and moduled for the first founder, it will be returned as it is(for example 1500). And then this 1500 position will be stored into tokenRecipient.

This means that founder have lost his vesting position, as when tokens are minted, then they are moduled to know if token should belong to founder.

So in case if i am first owner with 1% share that means that i will have nothing vested to me after migration, in case if reservedUntilTokenId > 99.

Note, this will affect only first founder and only 1 out of his total percents. All other founders will get correct postions.

Impact

Founder loses vesting nfts

Code Snippet

Provided above

Tool used

Manual Review

Recommendation

Change to this code.

-               uint256 baseTokenId = reservedUntilTokenId;
+              uint256 baseTokenId = reservedUntilTokenId % 100;

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

Duplicate of #42

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Aamirusmani1552 commented:

(Important:)I have created this class as I have couldn't focus on Migration Part of the audit. I joined the actual audit as well and there also I couldn't focus on it. I am not sure which part of it could make the actual problem. So all issues related to migration are put in this class. I marked the one that I think could be the actual issue as a best report. Please go through each one of them properly. Thanks.