unforgiven - founders vesting allocation doesn't get updated after reservedUntilTokenId gets updated

[sherlock-admin2]

unforgiven

high

founders vesting allocation doesn't get updated after reservedUntilTokenId gets updated

Summary

function _addFounders() use the value of the reservedUntilTokenId to calculate and set the values for founders vesting allocation, and those vesting are get removed in updateFounders() which should be based on the reservedUntilTokenId value too. the issue is that when the value of the reservedUntilTokenId is updated, the founders vesting allocation should be calculated again and in current implementation vesting allocation stay based on the old value of the reservedUntilTokenId which will cause two issue:

1. wrong vesting allocation based on old data.
2. problem when removing vesting allocation in the updateFounders() based on new value of reservedUntilTokenId while allocation is based on old value of reservedUntilTokenId.

Vulnerability Detail

This is part of the _addFounders() code, as you can see it uses the value of the reservedUntilTokenId to calculate vesting allocations for founders.

                // Used to store the base token id the founder will recieve
                uint256 baseTokenId = reservedUntilTokenId;

                // For each token to vest:
                for (uint256 j; j < founderPct; ++j) {
                    // Get the available token id
                    baseTokenId = _getNextTokenId(baseTokenId);

                    // Store the founder as the recipient
                    tokenRecipient[baseTokenId] = newFounder;

                    emit MintScheduled(baseTokenId, founderId, newFounder);

                    // Update the base token id
                    baseTokenId = (baseTokenId + schedule) % 100;
                }

This is updateFounders() code, the vesting allocations which are saved in the tokenRecipient[] mappings are removed when updating founders. so when removing the vesting allocations, code should use the value of the reservedUntilTokenId again to calculate vesting allocations an remove them. (currently there is another bug that baseTokenId doesn't start with reservedUntilTokenId, but to correctly remove the vesting allocations in the tokenRecipient[] map, code should do exactly like _addFounders() but instead of setting those slots it should delete those slots)

                // Used to reverse engineer the indices the founder has reserved tokens in.
                uint256 baseTokenId;

                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];
                    clearedTokenIds[baseTokenId] = true;

                    emit MintUnscheduled(baseTokenId, i, cachedFounder);

                    // Update the base token id
                    baseTokenId = (baseTokenId + schedule) % 100;
                }

This is setReservedUntilTokenId() code, as you can see code doesn't update vesting allocations for founders.

    function setReservedUntilTokenId(uint256 newReservedUntilTokenId) external onlyOwner {
        if (settings.mintCount > 0) {
            revert CANNOT_CHANGE_RESERVE();
        }
        if (settings.totalSupply > 0 && reservedUntilTokenId > newReservedUntilTokenId) {
            revert CANNOT_DECREASE_RESERVE();
        }
        reservedUntilTokenId = newReservedUntilTokenId;
        emit ReservedUntilTokenIDUpdated(newReservedUntilTokenId);
    }

so the POC for the issue is:

1. DAO founders deploy the new DAO with reservedUntilTokenId as RUT1.
2. code would set founders vesting allocations based on the RUT1.
3. after a while DAO would change the value of the reservedUntilTokenId and merkle root for reserved tokens. the new value would be RUT2.
4. right now the vesting allocations are based on RUT1 but the value of the reservedUntilTokenId is RUT2 and the vesting allocations are wrong.
5. if DAO wants to update the founders list then the logic that removes the vesting allocation would delete wrong slots in tokenAllocation[] mapping because it would calculate those slots based on the RUT2 (if the other bug get fixed) but vesting allocation is still based RUT1.

Impact

wrong vesting allocation after reservedUntilTokenId changes that is based on old value.
wrong vesting allocation causing NFT loss if updateFounders() is get called too which creates messy vesting allocation.

Code Snippet

https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/db232c649b425c36f5a93607c95cfdf0e5962b2f/nouns-protocol/src/token/Token.sol#L411-L427
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/db232c649b425c36f5a93607c95cfdf0e5962b2f/nouns-protocol/src/token/Token.sol#L160-L175
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/db232c649b425c36f5a93607c95cfdf0e5962b2f/nouns-protocol/src/token/Token.sol#L484-L503

Tool used

Manual Review

Recommendation

either update the vesting allocation when reservedUntilTokenId changes or make logic of vesting allocation independent of the reservedUntilTokenId.