dimulski - Reserving 100 or more tokens for claiming when a DAO is created results in the first founder not receiving % of the NFTs entitled to him.

[sherlock-admin2]

dimulski

medium

Reserving 100 or more tokens for claiming when a DAO is created results in the first founder not receiving % of the NFTs entitled to him.

Summary

When a DAO is created there are n number of tokens that can be reserved for a private sale, if the founders have decided to bootstrap the project in such a way, or mint NFTs to certain people on discount. When setting up the parameters for the DAO there is the option for different founders to be set whom in turn will receive certain percentage of all the minted NFTs, until their vesting period is over. NFTs grant voting power. For example Nouns DAO mints 1 of each 10 NFTs to the treasury, a single auction continues for a day. In another scenario auctions can also be held every 15 mins for example Lil Nouns. The problem arises if reservedUntilTokenId in Token.sol is set to 100 or more. NFTs are distributed to founders based on their ownership percentage. For example if the total ownership of all the founders is 10%, and regular users mint 100 NFTs, the total minted NFTs will be 110 and 10 NFTs will be distributed to the founders based on their percentage share. Keep in mind that creators of DAOs on the nounsbuilder protocol may choose to reserve a lot of tokens based on factors such as the duration of each auction, for example as mentioned above if auctions are held every 15 mins we can assume that the reserve tokens will be way above 100.

Note: This issue doesn't describe a malicious owner imputing parameters that will benefit him, or supplying a specific wrong parameter that triggers an edge case which in turns leads to a loss of value for the protocol or founders. This issue describes a scenario where an admin or governance enters a parameter that would be expected under normal conditions, and something is broken.

Vulnerability Detail

First add the following to Token.t.sol

import { console2 } from "forge-std/Test.sol";

In NounsBuilderTest.sol modify the following:

• At line 96 change percents[0] = 10; to percents[0] = 5;
• At line 131 change 0 to 100

To better illustrate what is happening in the _addFounders() function add the following to the Token.sol contract:

import { console2 } from "forge-std/Test.sol";

• At line 170 in the _addFounders() function add console2.log("From Token.sol _addFounders: ", baseTokenId);

Add the following test to Token.t.sol

    function test_FounderLosesNFTShare() public {
        deployMock();
        // Mint 500 tokens
        for (uint256 i = 0; i < 500; i++) {
            vm.prank(address(auction));
            token.mint();
        }

        console2.log("Number of reserved tokens: ", token.reservedUntilTokenId());
        console2.log("Total ownership percentage of founders: ", token.totalFounderOwnership());
        console2.log("Percentage of the first founder: ", token.getFounder(0).ownershipPct);
        console2.log("Percentage of the second founder: ", token.getFounder(1).ownershipPct);
        address walletFounder1 = token.getFounder(0).wallet;
        address walletFounder2 = token.getFounder(1).wallet;
        console2.log("Balance of the first founder: ", token.balanceOf(walletFounder1));
        console2.log("Balance of the compromised founder: ", token.balanceOf(walletFounder2));
        console2.log("Here is the total supply: ", token.totalSupply());
    }

Logs:
  From Token.sol _addFounders:  100
  From Token.sol _addFounders:  20
  From Token.sol _addFounders:  40
  From Token.sol _addFounders:  60
  From Token.sol _addFounders:  80
  From Token.sol _addFounders:  1
  From Token.sol _addFounders:  21
  From Token.sol _addFounders:  41
  From Token.sol _addFounders:  61
  From Token.sol _addFounders:  81
  Number of reserved tokens:  100
  Total ownership percentage of founders:  10
  Percentage of the first founder:  5
  Percentage of the second founder:  5
  Balance of the first founder:  22
  Balance of the compromised founder:  28
  Here is the total supply:  550

To run the test use: forge test -vvv --mt test_FounderLosesNFTShare

Impact

In the above POC we set the reservedUntilTokenId to 100, meaning that there will be 100 NFTs that can be minted by whitelisted users. We also set two founders both with 5% share of the total NFTs minted. As can be seen from the above POC the first founder receives much less NFTs than the second founder. The NFTs are minted by regular users, winning the auction for the certain NFT. It is possible that founders catch up on the fact that they are not receiving the correct amount of NFTs they are entitled to, and may try to fix this by either first calling setReservedUntilTokenId() function in order to set a different reservedUntilTokenId, and then calling the updateFounders() function, or directly calling the updateFounders() function with some parameters that will (e.g try and distribute an NFT to another address held by the founder that is not receiving the full % of NFTs entitled to him). The updateFounders() function is implemented incorrectly, but this is a separate issue. The setReservedUntilTokenId() function can only be called if there are no NFTs minted by normal users via winning an auction. Even if they find the correct parameters to execute some of the above mentioned flows which is highly unlikely, they would have still lost some of the NFTs entitled to them, until that moment. From the below _addFounders() function we can see that the first time uint256 baseTokenId = reservedUntilTokenId is reached, when reservedUntilTokenId is 100 as in the above provided POC baseTokenId will be equal to 100.

    function _addFounders(IManager.FounderParams[] calldata _founders, uint256 reservedUntilTokenId) internal {
        // Used to store the total percent ownership among the founders
        uint256 totalOwnership;

        uint8 numFoundersAdded = 0;

        unchecked {
            // For each founder:
            for (uint256 i; i < _founders.length; ++i) {
                // ...

                // Compute the vesting schedule
                uint256 schedule = 100 / founderPct;

                // Used to store the base token id the founder will recieve
                uint256 baseTokenId = reservedUntilTokenId; //@audit in the above provided POC this will be set to 100

                // For each token to vest:
                for (uint256 j; j < founderPct; ++j) {
                    // Get the available token id
                    baseTokenId = _getNextTokenId(baseTokenId); //@audit _getNExtTokenId is called with 100 as param

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

                    emit MintScheduled(baseTokenId, founderId, newFounder);

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

            //...
        }
    }

Then the function enters in a for loop and calls _getNextTokenId(baseTokenId) with 100 as a parameter. The while loop in _getNextTokenId() won't execute as this is the first call to it , and there is nothing set in the tokenRecipient mapping. Thus this function will return 100 as the _tokenId. In the _addFounders() function we then see that the tokenRecipient mapping is updated with the parameters of the founder: tokenRecipient[baseTokenId] = newFounder;

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

            return _tokenId;
        }
    }

When the Auction.sol contract creates an auction it calls the mint() function in the Token.sol contract

function mint() external nonReentrant onlyAuctionOrMinter returns (uint256 tokenId) {
        tokenId = _mintWithVesting(msg.sender);
    }

which in turn calls _mintWithVesting()

   function _mintWithVesting(address recipient) internal returns (uint256 tokenId) {
        // Cannot realistically overflow
        unchecked {
            do {
                // Get the next token to mint
                tokenId = reservedUntilTokenId + settings.mintCount++;

                // Lookup whether the token is for a founder, and mint accordingly if so
            } while (_isForFounder(tokenId));
        }

        // Mint the next available token to the recipient for bidding
        _mint(recipient, tokenId);
    }

which in turn calls _isForFounder in order to check if an NFT should be minted to a founder. baseTokenId is calculated by applying the modulo operator to the current _tokenId. As described above we have a tokenRecipient[100] mapping pointing to the registered founder parameters. The problem is that there is no positive integer that when divided by 100 has a remainder equal to 100 or more. Meaning that baseTokenId can't be equal to 100 and the first if condition will be triggered. When the correct flow would be to trigger the else if condition, and mint an NFT to the founder if he is still vesting. In the above POC when the ownership percentage of the founder is 5 he will lose 20% of the NFTs entitled to him.

    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:
        }
       // ...
    }

Code Snippet

   for (uint256 j; j < founderPct; ++j) {
        // Get the available token id
        baseTokenId = _getNextTokenId(baseTokenId); //@audit _getNExtTokenId is called with 100 as param

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

         emit MintScheduled(baseTokenId, founderId, newFounder);

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

Tool used

Manual Review & Foundry

Recommendation

In _addFounders() function consider applying the %100operation to baseTokenId before the first call to _getNextTokenId() function is made.

Duplicate of #42