qpzm - Token.updateFounders does not delete the old founder's vesting schedule.

[sherlock-admin]

qpzm

high

Token.updateFounders does not delete the old founder's vesting schedule.

Summary

Token.updateFounders does not delete the old founder's vesting schedule.

Vulnerability Detail

Add the following code in test/POC2.sol and run forge test --mt test_POC2 -vvv.
The test shows that if baseTokenId = (baseTokenId + schedule) % 100; rounds back to a smaller number than the first baseTokenId,
updateFounders does not delete the old founder's baseTokenIds.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.16;

import "./Token.t.sol";
import { console } from "forge-std/console.sol";

contract TokenTestPOC2 is TokenTest {
    function setUp() public virtual override {
        super.setUp();
        createDelegator();
    }

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

        setMockTokenParamsWithReserve(_reservedUntilTokenId);

        setMockAuctionParams();

        setMockGovParams();

        deploy(foundersArr, tokenParams, auctionParams, govParams);

        setMockMetadata();
    }

    function test_POC2() public {
        uint256 f1Percentage = 11;
        address f1Wallet = address(0x1);
        address f2Wallet = address(0x2);

        address[] memory founders = new address[](1);
        uint256[] memory percents = new uint256[](1);
        uint256[] memory vestingEnds = new uint256[](1);

        founders[0] = f1Wallet;
        percents[0] = f1Percentage;
        vestingEnds[0] = 4 weeks;

        // token 0 ~ 9 is reserved
        // @audit tokenId 10, 19, 28, 37, 46, 55, 64, 73, 82, 91, 0 is for founder f1Wallet
        uint256 reservedUntilTokenId = 10;
        deployWithCustomFounders(founders, percents, vestingEnds, reservedUntilTokenId);

        Founder memory f1 = token.getFounder(0);

        assertEq(f1.ownershipPct, f1Percentage);

        setFounderParams(founders, percents, vestingEnds);
        founders[0] = f2Wallet;
        vm.prank(f1Wallet);
        token.updateFounders(foundersArr);

        // @audit f1Wallet's vesting for tokenId 10, 19, 28, 37, 46, 55, 64, 73, 82, 91 is not deleted.
        for (uint256 i = 0; i < f1Percentage - 1; i++) {
            uint256 vestingTokenId = (reservedUntilTokenId + i * 100 / f1Percentage) % 100;
            assertEq(token.getScheduledRecipient(vestingTokenId).wallet, f1Wallet);
        }

        // @audit only tokenId 0 is deleted
        assertEq(token.getScheduledRecipient(0).wallet, address(0));
    }
}

When I add import { console } from "forge-std/console.sol"; at Token.sol and add console.log in addFounders and updateFounders, I can see the following logs.

1. _addFounders: add 10, 19, 28, ..., 91, 0
2. _updateFounders: delete 0, 9, 18, ..., 81, 90
3. _updaterFounders: add 11, 20, 29, ..., 92, 1

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

for (uint256 j; j < founderPct; ++j) {
    // Get the available token id
    baseTokenId = _getNextTokenId(baseTokenId);
    // @audit Add this line
    console.log("[_addFounders] baseTokenId: %s", baseTokenId);

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

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];
    // @audit Add this line
    console.log("[updateFounders] baseTokenId: %s", baseTokenId);
    clearedTokenIds[baseTokenId] = true;

$ forge test --mt test_POC2 -vvv

Logs:
  [_addFounders] baseTokenId: 10
  [_addFounders] baseTokenId: 19
  [_addFounders] baseTokenId: 28
  [_addFounders] baseTokenId: 37
  [_addFounders] baseTokenId: 46
  [_addFounders] baseTokenId: 55
  [_addFounders] baseTokenId: 64
  [_addFounders] baseTokenId: 73
  [_addFounders] baseTokenId: 82
  [_addFounders] baseTokenId: 91
  [_addFounders] baseTokenId: 0
  // This is deleted ids
  [updateFounders] baseTokenId: 0
  [updateFounders] baseTokenId: 9
  [updateFounders] baseTokenId: 18
  [updateFounders] baseTokenId: 27
  [updateFounders] baseTokenId: 36
  [updateFounders] baseTokenId: 45
  [updateFounders] baseTokenId: 54
  [updateFounders] baseTokenId: 63
  [updateFounders] baseTokenId: 72
  [updateFounders] baseTokenId: 81
  [updateFounders] baseTokenId: 90
  // This is newly added ids
  [_addFounders] baseTokenId: 11
  [_addFounders] baseTokenId: 20
  [_addFounders] baseTokenId: 29
  [_addFounders] baseTokenId: 38
  [_addFounders] baseTokenId: 47
  [_addFounders] baseTokenId: 56
  [_addFounders] baseTokenId: 65
  [_addFounders] baseTokenId: 74
  [_addFounders] baseTokenId: 83
  [_addFounders] baseTokenId: 92
  [_addFounders] baseTokenId: 1

Impact

The old founder can still get token vesting unexpectedly.

Code Snippet

https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L158-L176
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L414-L428

Tool used

Manual Review, foundry

Recommendation

There are two solutions.
First, iterate from not reservedUntilTokenId, but 0 in Token._addFounders and Token.updateFounders. This delays the founders' vesting a little.
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L161

-                uint256 baseTokenId = reservedUntilTokenId;
+                uint256 baseTokenId;

After the fix above, run forge test --mt test_POC2 -vvv again.

Logs:
  [_addFounders] baseTokenId: 0
  [_addFounders] baseTokenId: 9
  [_addFounders] baseTokenId: 18
  [_addFounders] baseTokenId: 27
  [_addFounders] baseTokenId: 36
  [_addFounders] baseTokenId: 45
  [_addFounders] baseTokenId: 54
  [_addFounders] baseTokenId: 63
  [_addFounders] baseTokenId: 72
  [_addFounders] baseTokenId: 81
  [_addFounders] baseTokenId: 90
  // The deleted ids are the same as ids added in _addFounders.
  [updateFounders] baseTokenId: 0
  [updateFounders] baseTokenId: 9
  [updateFounders] baseTokenId: 18
  [updateFounders] baseTokenId: 27
  [updateFounders] baseTokenId: 36
  [updateFounders] baseTokenId: 45
  [updateFounders] baseTokenId: 54
  [updateFounders] baseTokenId: 63
  [updateFounders] baseTokenId: 72
  [updateFounders] baseTokenId: 81
  [updateFounders] baseTokenId: 90
  // The below ids are reserved for the new founder.
  [_addFounders] baseTokenId: 0
  [_addFounders] baseTokenId: 9
  [_addFounders] baseTokenId: 18
  [_addFounders] baseTokenId: 27
  [_addFounders] baseTokenId: 36
  [_addFounders] baseTokenId: 45
  [_addFounders] baseTokenId: 54
  [_addFounders] baseTokenId: 63
  [_addFounders] baseTokenId: 72
  [_addFounders] baseTokenId: 81
  [_addFounders] baseTokenId: 90

Second, if the logic becomes too complex, just iterate from 0 to 99 and delete if tokenRecipient[baseTokenId].wallet != address(0).

Duplicate of #42