dany.armstrong90 - Token.sol#updateFounders: The initial value of baseTokenId is set wrong.

[sherlock-admin2]

dany.armstrong90

high

Token.sol#updateFounders: The initial value of baseTokenId is set wrong.

Summary

The initial value of baseTokenId is zero in Token.sol#updateFounders function and it differs from the initial value in Token.sol#_addFounders function.
As a result, the tokenRecipient state variables may not be deleted and tokens are minted to the founders who have been removed.
In extreme cases, minting token will not be possible at all.

Vulnerability Detail

The Token.sol#updateFounders function is as follows.

File: Token.sol
375:     function updateFounders(IManager.FounderParams[] calldata newFounders) external onlyOwner {
376:         // Cache the number of founders
377:         uint256 numFounders = settings.numFounders;
378: 
379:         // Get a temporary array to hold all founders
380:         Founder[] memory cachedFounders = new Founder[](numFounders);
381: 
382:         // Cannot realistically overflow
383:         unchecked {
384:             // Add each founder to the array
385:             for (uint256 i; i < numFounders; ++i) {
386:                 cachedFounders[i] = founder[i];
387:             }
388:         }
389: 
390:         // Keep a mapping of all the reserved token IDs we're set to clear.
391:         bool[] memory clearedTokenIds = new bool[](100);
392: 
393:         unchecked {
394:             // for each existing founder:
395:             for (uint256 i; i < cachedFounders.length; ++i) {
396:                 // copy the founder into memory
397:                 Founder memory cachedFounder = cachedFounders[i];
398: 
399:                 // Delete the founder from the stored mapping
400:                 delete founder[i];
401: 
402:                 // Some DAOs were initialized with 0 percentage ownership.
403:                 // This skips them to avoid a division by zero error.
404:                 if (cachedFounder.ownershipPct == 0) {
405:                     continue;
406:                 }
407: 
408:                 // using the ownership percentage, get reserved token percentages
409:                 uint256 schedule = 100 / cachedFounder.ownershipPct;
410: 
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:                 }
428:             }
429:         }
430: 
431:         // Clear values from storage before adding new founders
432:         settings.numFounders = 0;
433:         settings.totalOwnership = 0;
434:         emit FounderAllocationsCleared(newFounders);
435: 
436:         _addFounders(newFounders, reservedUntilTokenId);
437:     }

Thus baseTokenId is initialized to 0 in line 412.
On the other hand, The code of Token.sol#_addFounders function is the following.

File: Token.sol
120:     function _addFounders(IManager.FounderParams[] calldata _founders, uint256 reservedUntilTokenId) internal {
121:         // Used to store the total percent ownership among the founders
122:         uint256 totalOwnership;
123: 
124:         uint8 numFoundersAdded = 0;
125: 
126:         unchecked {
127:             // For each founder:
128:             for (uint256 i; i < _founders.length; ++i) {
129:                 // Cache the percent ownership
130:                 uint256 founderPct = _founders[i].ownershipPct;
131: 
132:                 // Continue if no ownership is specified
133:                 if (founderPct == 0) {
134:                     continue;
135:                 }
136: 
137:                 // Update the total ownership and ensure it's valid
138:                 totalOwnership += founderPct;
139: 
140:                 // Check that founders own less than 100% of tokens
141:                 if (totalOwnership > 99) {
142:                     revert INVALID_FOUNDER_OWNERSHIP();
143:                 }
144: 
145:                 // Compute the founder's id
146:                 uint256 founderId = numFoundersAdded++;
147: 
148:                 // Get the pointer to store the founder
149:                 Founder storage newFounder = founder[founderId];
150: 
151:                 // Store the founder's vesting details
152:                 newFounder.wallet = _founders[i].wallet;
153:                 newFounder.vestExpiry = uint32(_founders[i].vestExpiry);
154:                 // Total ownership cannot be above 100 so this fits safely in uint8
155:                 newFounder.ownershipPct = uint8(founderPct);
156: 
157:                 // Compute the vesting schedule
158:                 uint256 schedule = 100 / founderPct;
159: 
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:                 }
176:             }
177: 
178:             // Store the founders' details
179:             settings.totalOwnership = uint8(totalOwnership);
180:             settings.numFounders = numFoundersAdded;
181:         }
182:     }

Thus baseTokenId is initialized to reservedUntilTokenId in line 161 of this function.
So when reservedUntilTokenId % 100 != 0, the values of baseTokenId will differ between the two functions.
As a result, the tokenRecipient state variables may not be deleted and the removed founders may be allocated tokens as ever.

Example:

1. Suppose that _founders.length == 2, _founders[0].ownershipPct = 2, _founders[1].ownershipPct = 2 and reservedUntilTokenId == 120.
2. The result of _addFounders function is that tokenRecipient[120] = tokenRecipient[70] = _founders[0] and tokenRecipient[21] = tokenRecipient[71] = _founders[1].
3. On the other hand, the values of tokenRecipient[0,1,50,51] will be deleted in updateFounders function and the values of tokenRecipient[120,70,21,71] will never be deleted.
4. Following two cases are available.

1. Suppose that newFounders.length == 1 and newFounders[0].ownershipPct = 1 in updateFounders function. Then the values of tokenRecipient[70,21,71] will never be overwritten and the removed founders may be allocated tokens as ever.
2. Suppose that newFounders.length == 1 and newFounders[0].ownershipPct = 97 in updateFounders function. Then tokenRecipient[baseTokenId] != address(0) for all 0 <= baseTokenId < 100 will hold. After that, call to _mintWithVesting function will fall in infinite loop until the gas is exhausted entirely and will revert. Thus mint token is not possible at all.

Impact

After the list of founders is updated with reservedUntilTokenId % 100 != 0, tokens is minted to the founders who have been removed.
In extreme cases, minting token will not be possible at all.

Code Snippet

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

Tool used

Manual Review

Recommendation

Modify Token.sol#L412 equal to Token.sol#L161.

Duplicate of #42