rvierdiiev - Token.updateFounders will not clear tokenRecipient correctly

[sherlock-admin2]

rvierdiiev

medium

Token.updateFounders will not clear tokenRecipient correctly

Summary

Token.updateFounders function doesn't initialize baseTokenId as reservedUntilTokenId during position clearing for the owner. Because of incorrect offset it will be impossible to clear positions correctly, which leads to bigger amount of tokens for same owner.

Vulnerability Detail

When new Token is created, then _addFounders is called.

This function loops through the founders and fetches their percentage of minted tokens during vesting. Then function populates tokenRecipient mapping to mark token ids that should belong to owner(tokenId % 100).
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L164-L175

                uint256 baseTokenId = reservedUntilTokenId;
                
                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;
                }

In case if owner has 20% ownership that means that he will receive 20 out of 100 minted tokens during the vesting.
One thing that we should note here is that baseTokenId is initialized with reservedUntilTokenId which is the amount of tokens that are reserved. As result tokenRecipient values will be shifted by reservedUntilTokenId.

Using updateFounders function owner have ability to update founders.
First thing function is trying to clear tokenRecipient mapping, so all old owner's vesting positions are cleared.

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

                uint256 baseTokenId;

                for (uint256 j; j < cachedFounder.ownershipPct; ++j) {
                    // Get the next index that hasn't already been cleared
                    while (clearedTokenIds[baseTokenId] != false) {
                        baseTokenId = (++baseTokenId) % 100;
                    }

                    delete tokenRecipient[baseTokenId];
                    clearedTokenIds[baseTokenId] = true;

                    emit MintUnscheduled(baseTokenId, i, cachedFounder);

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

This function is trying to clear all positions and mark them as cleared.
This code is really similar to the one that is used in _addFounders function, however baseTokenId is 0 here and is not initialized with reservedUntilTokenId.

Because baseTokenId is not initialized with reservedUntilTokenId, it's likely that all positions in the tokenRecipient will not be cleared as wrong token position will be assumed to belong to owner.

Example(the most simple):

• reservedUntilTokenId = 10, and we have 1 owner with 1% ownership, so tokenRecipient[10] = owner1
• now updateFounders is called and because baseTokenId is initialized as 0, then tokenRecipient[10] was not cleared
• same owner was set with percentage 1%(same percentage, so it's easier to explain), then tokenRecipient[11] = owner1 and tokenRecipient[10] = owner1 still remains

Now owner1 has 2 tokens out of 100 instead of 1.
The problem is that in case if user has 1% then he receives only 1 additional token, but in case if he had bigger percentage, then he will get bigger amount of additional tokens.

This bug can be triggered in case if dao uses reservedUntilTokenId or have migrated from l1 to l2(as it's likely that reservedUntilTokenId will not be 0 only after migration) and after that updateFounders was called.

Impact

Owner receives additional vesting tokens.

Code Snippet

Provided above

Tool used

Manual Review

Recommendation

Initialize baseTokenId with reservedUntilTokenId value.

Duplicate of #42

[sherlock-admin2]

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

Aamirusmani1552 commented:

This should be a valid issue as there is no function in the contract itself that transfers the properties of NFT to the other metadata renderer. And if it is not done then the users will surely loose their NFTs. This is marked as medium as it is very unlikely that the update for the existing metadata renderer will be done. But still it is a valid issue according to me.