KingNFT - Founders could still obtain vesting NFTs even when they have been intentionally removed by the contract owner

[sherlock-admin2]

KingNFT

high

Founders could still obtain vesting NFTs even when they have been intentionally removed by the contract owner

Summary

The incorrect implementation of updateFounders() would cause that founders could still claim vesting NFTs even when they have been intentionally removed by the contract owner.

Vulnerability Detail

The issue arises on L412, the baseTokenId should begin with reservedUntilTokenId % 100.

File: src\token\Token.sol
375:     function updateFounders(IManager.FounderParams[] calldata newFounders) external onlyOwner {
...
393:         unchecked {
394:             // for each existing founder:
395:             for (uint256 i; i < cachedFounders.length; ++i) {
...
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; // @audit should begin with reservedUntilTokenId
+412:                 uint256 baseTokenId = reservedUntilTokenId % 100;
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:         }
...
437:     }

let's say the reservedUntilTokenId == 10 and founderPct = 5, when _addFounders() in initialize(), the following tokenRecipients would be set.

tokenRecipient[10]
tokenRecipient[30]
tokenRecipient[50]
tokenRecipient[70]
tokenRecipient[90]

But in updateFounders(), as baseTokenId begin with 0, the actual cleared tokenRecipients are

tokenRecipient[0]
tokenRecipient[20]
tokenRecipient[40]
tokenRecipient[60]
tokenRecipient[80]

We can see, none of previous valid tokenRecipient info is cleared. Hence, founders could still obtain those vesting NFTs.

Impact

Founders could still claim vesting NFTs even when they have been intentionally removed.

Code Snippet

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

Tool used

Manual Review

Recommendation

see Vulnerability Detail

Duplicate of #42

[ydspa]

Escalate

This is not dup of #42, with both different affected function and different impact.

[sherlock-admin2]

Escalate

This is not dup of #42, with both different affected function and different impact.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

Will review all duplicates under #42, so please check deduplication comments here in the mean time.

[nevillehuang]

See comment here

[Czar102]

I agree with the Lead Judge, planning to leave the issue as is. @ydspa please let me know if the logic in the above comment makes sense.

[Czar102]

Result:
High
Duplicate of #42

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ydspa: rejected