Falconhoof - First founder receives less token allocation than expected

[sherlock-admin2]

Falconhoof

high

First founder receives less token allocation than expected

See Token.sol here
See Token.sol::_getNextTokenId() here
See Token.sol::_isForFounder() here

Summary

If reservedUntilTokenId is set to any three digit number (e.g. 100); the first founder in the founders array loses 1% of token supply during their vesting period.

Vulnerability Detail

Token.sol::_addFounders() takes in two parameters; _foundersand reservedUntilTokenId. The function loops through each founder in the _founders array assigning tokens based on percentages due.

For each founder the starting point for assigning their tokens is reservedUntilTokenId:

        uint256 baseTokenId = reservedUntilTokenId;

For all except the first token checked in _getNextTokenId, see here, the input token ID is modulated.
This means that _getNextTokenId returns an unmodulated token ID from the tokenRecipient for all except the very first token checked; unmodulated meaning a value between0 - 99.

This returned token is then assigned to that user in the tokenRecipient mapping in _addFounders

         tokenRecipient[baseTokenId] = newFounder;

During the minting phase; assignments in the tokenRecipient mapping are checked to determine, via `Token.sol::_isForFounder()', whether a token can be used for an auction or should be minted to a founder; see here.

The problem with allocating the first token to the initial founder arises because the check in _getNextTokenId does not include modulation. Consequently, this leads to the function returning false for the condition:

         while (tokenRecipient[_tokenId].wallet != address(0)) {

This allows any provided value to be directly returned and stored in tokenRecipient. This behavior does not present a problem for token IDs that are single or double-digit numbers. H
owever, if this value has three digits i.e. reservedUntilTokenId = 100; it will be stored unchanged in the tokenRecipient mapping.

Impact

The first founder in the _founders array loses a significant amount of voting power and tokens. It will also throw the voting dynamics of the DAO into disarray.

Using as an example reservedUntilTokenId = 100 and the first founder allocation percentage of 1 ; the first founder should be entitled to 1% of all tokens minted from 100 onwards within their vesting period, e.g. Token IDs 100, 200, 300, 400, etc. However they would receive nothing.

Code Snippet

Proof of Concept

Add the following function to NounsBuilderTest.sol:

    function deployWithCustomFoundersAndReserveToken(
        address[] memory _wallets,
        uint256[] memory _percents,
        uint256[] memory _vestExpirys,
        uint256 _reservedUntilTokenId
    ) internal virtual {
        setFounderParams(_wallets, _percents, _vestExpirys);

        setMockTokenParamsWithReserve(_reservedUntilTokenId);

        setMockAuctionParams();

        setMockGovParams();

        deploy(foundersArr, tokenParams, auctionParams, govParams);

        setMockMetadata();
    }

Add the following test to Token.t.sol and run:

    function test_firstFounderAllocationLoss() public {

        uint256[] memory percentages = new uint256[](1);
        address[] memory founders = new address[](percentages.length);
        uint256[] memory vesting = new uint256[](percentages.length);

        for (uint256 i = 0; i < percentages.length; i++) {

            // Set founders[0] percentage to 1%
            percentages[i] = 1;
            founders[i] = founder;
            vesting[i] = 4 weeks;
        }

        uint256 reservedUntilTokenId = 100;

        vm.prank(founder);
        deployWithCustomFoundersAndReserveToken(founders, percentages, vesting, reservedUntilTokenId);

        // unpause mints first token for auction
        // it should be 101 as 100 should be minted for founder
        vm.prank(founder);
        auction.unpause();

        // assert first auction token is 100
        (uint256 tokenId, , , , , ) = auction.auction();
        assertEq(tokenId, 100);

        // Loop 500 times; we expect founder to get 1% i.e. 5 tokens
        for (uint256 i = 0; i < 500; i++) {
            vm.prank(address(auction));
            token.mint();
        }

        // Assert founder receives no tokens out of first 500 mints
        uint256 founderBalance = token.balanceOf(founder);
        assertEq(founderBalance, 0);
    }

Tool used

Foundry
Manual Review

Recommendation

In _getNextTokenId, modulate _tokenId before entering the while loop:

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

Duplicate of #42