ge6a - If reservedUntilTokenId > 100, the first founder will receive fewer tokens than expected

[sherlock-admin]

ge6a

high

If reservedUntilTokenId > 100, the first founder will receive fewer tokens than expected

Summary

The addFounders function is used to define a scheme for vesting tokens for founders. This is done by iterating through the founders, and for each ownership percentage, a record is added to the tokenRecipient map. The keys of this map should be remainders when divided by 100. For example, tokenRecipient[x] = newFounder means that newFounder should receive 1 of every 100 tokens during the vesting period. However, there is an issue in the logic where, if reservedUntilTokenId > 100, the first founder in the array of founders will have one less record in tokenRecipient than there should be. This means that they will receive 1% fewer tokens (from all tokens) than they should. For instance, if ownershipPct=50, they will actually receive 49% of the tokens instead of 50.

Vulnerability Detail

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

This issue arises because the initial value of baseTokenId is set to reservedUntilTokenId, instead of looking at the remainder when divided by 100, as is done in the subsequent iterations. This way, a key > 100 is recorded in tokenRecipient, for which tokens cannot be vested in the _isForFounder() function. I created a POC to demonstrate the issue, you can add it into Token.t.sol:

function testBug() public
{
        setMockFounderParams();
        setMockTokenParamsWithReserve(1005);
        setMockAuctionParams();
        setMockGovParams();
        deploy(foundersArr, tokenParams, auctionParams, govParams);
        setMockMetadata();

        console.log("Address: %s", token.getScheduledRecipient2(1005).wallet); // we have value for 1005 but shouldn't
}

//Token.sol
function getScheduledRecipient2(uint256 _tokenId) external view returns (Founder memory) {
      return tokenRecipient[_tokenId];
}

Impact

The first founder will receive significantly less tokens than expected which is a financial loss. The likelihood to have reservedUntilTokenId > 100 is High.

Code Snippet

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

Tool used

Manual Review

Recommendation

Use reservedUntilTokenId % 100 on line 161 in Token.sol

Duplicate of #42