Nouns Builder contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L69-L126 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L128-L137

Vulnerability details

Impact

function _addFounders() is Called upon initialization to add founders and compute their vesting allocations and it uses _getNextTokenId() to finds the next available base token id for a founder, but for some cases of founder's percentages the logics won't work and code would try to set value for tokenRecipient[100] which would cause some founders to not get the percentages that specified for them. this would cause fund loss for some founders and wrong token distribution. the is a logical bug which can cause serious fund loss.

Proof of Concept

This is part of _addFounders() code that loops through and specify which tokens(base on %100) is going to be assigned to one founder:

                // 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 += schedule) % 100;
                }

As you can see code calls baseTokenId = _getNextTokenId(baseTokenId); and the return value of _getNextTokenId() is used in tokenRecipient[baseTokenId] = newFounder; to set values in tokenRecipient[]. there are some scenarios that function _getNextTokenId() returns value grater than 99. This is _getNextTokenId() code:

    /// @dev Finds the next available base token id for a founder
    /// @param _tokenId The ERC-721 token id
    function _getNextTokenId(uint256 _tokenId) internal view returns (uint256) {
        unchecked {
            while (tokenRecipient[_tokenId].wallet != address(0)) ++_tokenId;

            return _tokenId;
        }
    }

As you can see This functions increase tokenId until it finds a token that is not assigned to anyone. there are scenarios that this function returns a number greater than 99 and then code would set value for tokenRecipient[100] that would cause the logics of token distribution to be failed totally because when tokens are distributing, code use % 100 to find the owner of that token and index 100 would never get any token. This is one sample that logics of code won't work for it:

1. first 5 founder with 1% in list, (so indexes: 0, 1, 2, 3, 4 would be filled in tokenRecipient[])
2. then 5 founder with 10%, (so indexes: x5, x6, x7, x8, x9 would be filled fo x=[0,1,2,....,9] in tokenRecipient[], specially 95,96,97,98,99 would be filled)
3. then 1 founder with 20%, (here code would start from index 10 (becasue 0 to 9 is filled) and increase 5 by 5 to fill indexes (10, 15, 20, 25, ..., 90, 95), in each x5 index because x5, x6, x7, x8, x9 are filled in step 2 so _getNextTokenId() code would increase index to find a free slot, when it reaches to 95, _getNextTokenId() would return 100 and code set value for tokenRecipient[100])

As mentioned before when tokenRecipient[100] is filled then those tokens would not be assigned to founders because in assigning tokens code uses ``tokenRecipient[index % 100].walletto find the owner of the new token and100could never be result ofindex % 100` and some founders would not get the percentages that is defined for them and they would lose their tokens.

There are other cases that this bug would happen to, this is a critical bug because logics of code don't support all cases of founder's percentages and after deployment for some cases code would work and users would lose funds.

Tools Used

VIM

Recommended Mitigation Steps

make sure that _getNextTokenId() don't return number greater than 99.