KupiaSec - The founder allocation logic will be broken after updating founders when reservedUntilTokenId > 0.

[sherlock-admin]

KupiaSec

high

The founder allocation logic will be broken after updating founders when reservedUntilTokenId > 0.

Summary

While updating the founders with positive reservedUntilTokenId, it doesn't remove the previous recipicent mapping properly and it might have more founder ownership than expected.

Vulnerability Detail

updateFounders() is used to update founders by an owner.

File: nouns-protocol\src\token\Token.sol
412:                 uint256 baseTokenId; //@audit starts from 0
413: 
414:                 for (uint256 j; j < cachedFounder.ownershipPct; ++j) {
415:                     // Get the next index that hasn't already been cleared
416:                     while (clearedTokenIds[baseTokenId] != false) {
417:                         baseTokenId = (++baseTokenId) % 100;
418:                     }
419: 
420:                     delete tokenRecipient[baseTokenId];
421:                     clearedTokenIds[baseTokenId] = true;
422: 
423:                     emit MintUnscheduled(baseTokenId, i, cachedFounder);
424: 
425:                     // Update the base token id
426:                     baseTokenId = (baseTokenId + schedule) % 100;
427:                 }

While removing the previous founders, it uses the same mechanism as _addFounders() but baseTokenId starts from 0, not reservedUntilTokenId.

As a result, it will remove wrong token IDs and some recipients wouldn't be cleared at all if reservedUntilTokenId > 0.

Here is a coded POC which can be added to Token.t.sol.

    function test_IncorrectUpdateFoundersWithPositiveReserveId() public {
        uint256 reservedUntilTokenId = 1;
        deployAltMock(reservedUntilTokenId); // add initial founders with reservedUntilTokenId = 1

        IManager.FounderParams[] memory newFoundersArr = new IManager.FounderParams[](1);
        newFoundersArr[0] = IManager.FounderParams({
            wallet: address(0x06B59d0b6AdCc6A5Dc63553782750dc0b41266a3),
            ownershipPct: 50,
            vestExpiry: 2556057600
        });

        vm.prank(address(founder));
        token.updateFounders(newFoundersArr); // update founders

        uint256 expectedTotalOwnership = token.totalFounderOwnership();
        assertEq(expectedTotalOwnership, 50); // expectedOwnership = 50

        Founder memory thisFounder;

        uint actualTotalOwnership;
        for (uint256 i; i <= 99; ++i) {
            thisFounder = token.getScheduledRecipient(i);
            actualTotalOwnership += thisFounder.wallet != address(0) ? 1: 0;
        }

        assertGt(actualTotalOwnership, expectedTotalOwnership);
        assertEq(actualTotalOwnership, expectedTotalOwnership + 10); // actualOwnership = 60
    }

After updating founders, it has 60% ownership instead of 50%.

Impact

The founder allocation logic will be broken after updating founders if reservedUntilTokenId > 0.
Although the owner knows about that, he can't mitigate with the current implementation of updateFounders().

Code Snippet

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

Tool used

Manual Review

Recommendation

The purpose of this logic is to remove previous ownership settings.

So we can just delete tokenRecipient mapping from 0 to 99.

for (uint256 j; j < 100; ++j) {
    if (tokenRecipient[j].ownershipPct > 0) {
        delete tokenRecipient[j];
    }
}

Duplicate of #42