ydlee - In updateFounders, the computation of owner's reserved token indices is different from that in _addFounders, resulting the old founders not be completely removed.

[sherlock-admin]

ydlee

high

In updateFounders, the computation of owner's reserved token indices is different from that in _addFounders, resulting the old founders not be completely removed.

Summary

In updateFounders, the computation of owner's reserved token indices is different from that in _addFounders. As a result, the reserved token indices for old founders may not be completely removed, and old founders may still get new vesting tokens.

Vulnerability Detail

The computation of owner's reserved token indices different between _addFounders and updateFounders. In _addFounders, it starts with reservedUntilTokenId (L161), while in updateFounders, it starts with 0 (L412).

// Function: Token.sol#_addFounders

160:                // Used to store the base token id the founder will recieve
161:->              uint256 baseTokenId = reservedUntilTokenId;
162:
163:                // For each token to vest:
164:                for (uint256 j; j < founderPct; ++j) {
165:                    // Get the available token id
166:->                  baseTokenId = _getNextTokenId(baseTokenId);
167:
168:                    // Store the founder as the recipient
169:->                  tokenRecipient[baseTokenId] = newFounder;
170:
171:->                  emit MintScheduled(baseTokenId, founderId, newFounder);
172:
173:                    // Update the base token id
174:->                  baseTokenId = (baseTokenId + schedule) % 100;
175:                }

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

// Function: Token.sol#updateFounders

411:                // Used to reverse engineer the indices the founder has reserved tokens in.
412:->              uint256 baseTokenId;
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:                }

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

We can check the difference from the event log of MintScheduled in _addFounders (L171), and MintUnscheduled in updateFounders (L423).

Let's see it with the existing test case Token.t.sol#test_UpdateFoundersZeroOwnership. We first set reservedUntilTokenId to 17, and then run the test case with forge test -vvvvv --match-test test_UpdateFoundersZeroOwnership.

123:    function setMockTokenParams() internal virtual {
124:        setTokenParams(
125:            "Mock Token",
126:            "MOCK",
127:            "This is a mock token",
128:            "ipfs://Qmew7TdyGnj6YRUjQR68sUJN3239MYXRD8uxowxF6rGK8j",
129:            "https://nouns.build",
130:            "http://localhost:5000/render",
131:->          17,  // set "reservedUntilTokenId" to 17
132:            address(0)
133:        );
134:    }

https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/test/utils/NounsBuilderTest.sol#L123-L134

The output is as follows. It is clear that baseTokenId in MintScheduled is different from that in MintUnscheduled.

    │   │   │   ├─ [593049] Token::initialize ......
    │   │   │   │   ├─ emit OwnerUpdated(prevOwner: 0x0000000000000000000000000000000000000000, newOwner: FOUNDER: [0xd3562Fd10840f6bA56112927f7996B7c16edFCc1])
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 17, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 27, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 37, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 47, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 57, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 67, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 77, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 87, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 97, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 7, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 18, founderId: 1, founder: (0xA7cBf566E80C4A1Df2C4aE965c79FB087f25E4bF, 5, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 38, founderId: 1, founder: (0xA7cBf566E80C4A1Df2C4aE965c79FB087f25E4bF, 5, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 58, founderId: 1, founder: (0xA7cBf566E80C4A1Df2C4aE965c79FB087f25E4bF, 5, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 78, founderId: 1, founder: (0xA7cBf566E80C4A1Df2C4aE965c79FB087f25E4bF, 5, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit MintScheduled(baseTokenId: 98, founderId: 1, founder: (0xA7cBf566E80C4A1Df2C4aE965c79FB087f25E4bF, 5, 2419200 [2.419e6]))
    │   │   │   │   ├─ emit Initialized(version: 1)
    │   │   │   │   └─ ← ()

    ├─ [317664] TOKEN::updateFounders([(0x06B59d0b6AdCc6A5Dc63553782750dc0b41266a3, 0, 2556057600 [2.556e9]), (0x06B59d0b6AdCc6A5Dc63553782750dc0b41266a3, 10, 2556057600 [2.556e9])])
    │   ├─ [317242] Token::updateFounders([(0x06B59d0b6AdCc6A5Dc63553782750dc0b41266a3, 0, 2556057600 [2.556e9]), (0x06B59d0b6AdCc6A5Dc63553782750dc0b41266a3, 10, 2556057600 [2.556e9])]) [delegatecall]
    │   │   ├─ emit MintUnscheduled(baseTokenId: 0, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 10, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 20, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 30, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 40, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 50, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 60, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 70, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 80, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 90, founderId: 0, founder: (0xd3562Fd10840f6bA56112927f7996B7c16edFCc1, 10, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 1, founderId: 1, founder: (0xA7cBf566E80C4A1Df2C4aE965c79FB087f25E4bF, 5, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 21, founderId: 1, founder: (0xA7cBf566E80C4A1Df2C4aE965c79FB087f25E4bF, 5, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 41, founderId: 1, founder: (0xA7cBf566E80C4A1Df2C4aE965c79FB087f25E4bF, 5, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 61, founderId: 1, founder: (0xA7cBf566E80C4A1Df2C4aE965c79FB087f25E4bF, 5, 2419200 [2.419e6]))
    │   │   ├─ emit MintUnscheduled(baseTokenId: 81, founderId: 1, founder: (0xA7cBf566E80C4A1Df2C4aE965c79FB087f25E4bF, 5, 2419200 [2.419e6]))

Impact

In updateFounders, the computation of owner's reserved token indices is different from that in _addFounders. As a result, the reserved token indices for old founders may not be completely removed, and old founders may still get new vesting tokens.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

In updateFounders function, set the initial value of baseTokenId to reservedUntilTokenId.

@@ -409,7 +409,7 @@ contract Token is IToken, VersionedContract, UUPS, Ownable, ReentrancyGuard, ERC
                 uint256 schedule = 100 / cachedFounder.ownershipPct;
 
                 // Used to reverse engineer the indices the founder has reserved tokens in.
-                uint256 baseTokenId;
+                uint256 baseTokenId = reservedUntilTokenId;
 
                 for (uint256 j; j < cachedFounder.ownershipPct; ++j) {
                     // Get the next index that hasn't already been cleared

Duplicate of #42