cawfree - Invariant Violation: Token.sol#_updateFounders does not respect reservedUntilTokenId.

[sherlock-admin2]

cawfree

medium

Invariant Violation: Token.sol#_updateFounders does not respect reservedUntilTokenId.

Summary

When adding new founders to a Token.sol, it is possible to define a reservedUntilTokenId to define what index auctions will start at.

However, when we make a call to updateFounders, it does not take into account the offset correctly and will fail to remove existing founders. This leads to unintentional founder mints and direct subversion of the protocol's maximum limit on founder ownership.

Vulnerability Detail

When initializing the mapping of founders via a call to _addFounders, the token distribution schedule is calculated taking into account a reservedUntilTokenId offset. When this value is non-zero, this has the effect of shifting the address generation schedule by an equivalent number of tokenIds.

For visual demonstration, for a single founder with 1% of ownership and no reserved tokens (Sequence A), the following token distribution pattern is used:

Token ID	Founder Reservation Address
1	0x0000000000000000000000000000000000000069
2	0x0000000000000000000000000000000000000000
...	...
101	0x0000000000000000000000000000000000000069
102	0x0000000000000000000000000000000000000000

Next, let's imagine a founder with 1% ownership and a single reserved token (Sequence B):

Token ID	Founder Reservation Address
1	0x0000000000000000000000000000000000000000
2	0x0000000000000000000000000000000000000069
...	...
101	0x0000000000000000000000000000000000000000
102	0x0000000000000000000000000000000000000069

Notice the generation sequence is effectively reversed.

Now, when making a call to updateFounders, the reservedUntilTokenId is not taken into account when trying to predict the previously allocated addresses. In this regard, when trying to predict the token identifiers possess founder addresses which must to be reset for Sequence B, the codebase mistakenly computes Sequence A instead.

As a result, if we try to assign token distribution Sequence B with Sequence A via a call to updateFounders, the resultant token recipients are as follows:

Token ID	Founder Reservation Address (Perpetual Vesting)
1	0x0000000000000000000000000000000000000069
2	0x0000000000000000000000000000000000000069
...	0x0000000000000000000000000000000000000069
101	0x0000000000000000000000000000000000000069
102	0x0000000000000000000000000000000000000069
...	...
type(uint256).max	0x0000000000000000000000000000000000000069

Notice that this has the effect of completely ignoring any of the previously reserved tokenIds whilst trying to purge the old founder configuration.

This can be reproduced in Foundry using the following test:

function test_sherlock_orphanedFounders() public {
    address[] memory wallets = new address[](1);
    uint256[] memory percents = new uint256[](1);
    uint256[] memory vestingEnds = new uint256[](1);

    address founder_a = address(0x94);

    wallets[0] = founder_a;

    percents[0] = 50;

    vestingEnds[0] = 4 weeks;

    setFounderParams(wallets, percents, vestingEnds);

    setTokenParams(
        "Mock Token",
        "MOCK",
        "This is a mock token",
        "ipfs://Qmew7TdyGnj6YRUjQR68sUJN3239MYXRD8uxowxF6rGK8j",
        "https://nouns.build",
        "http://localhost:5000/render",
        1, /// @audit
        address(0)
    );

    setMockAuctionParams();

    setMockGovParams();

    deploy(foundersArr, tokenParams, auctionParams, govParams);

    setMockMetadata();

    console.log("Before:");

    for (uint256 i = 0; i < 100;) {
        TokenTypesV1.Founder memory founder = token.getScheduledRecipient(i);
        console.log(founder.wallet);
        unchecked { ++i; }
    }

    address founder_b = address(0x97);

    wallets[0] = founder_b;

    percents[0] = 50;

    vestingEnds[0] = 4 weeks;

    delete foundersArr;

    setFounderParams(wallets, percents, vestingEnds);

    vm.prank(token.owner());
    token.updateFounders(foundersArr);

    console.log("After:");

    for (uint256 i = 0; i < 100;) {
        TokenTypesV1.Founder memory founder = token.getScheduledRecipient(i);
        console.log(founder.wallet);
        unchecked { ++i; }
    }

    assertEq(token.totalFounders(), 1);
}

Results in the following console output:

➜  nouns-protocol git:(main) ✗ forge test --match-contract "Sherlock" -vv
[⠊] Compiling...
[⠊] Compiling 1 files with 0.8.16
[⠒] Solc 0.8.16 finished in 4.02s
Compiler run successful!

Running 1 test for test/Token.Sherlock.t.sol:TokenSherlockTest
[PASS] test_sherlock_orphanedFounders() (gas: 5928101)
Logs:
  Before:
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000000
  0x0000000000000000000000000000000000000094
  After:
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094
  0x0000000000000000000000000000000000000097
  0x0000000000000000000000000000000000000094

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.97ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Notice here that both address(0x94) and address(0x97) are rewarded as founders after the call to updateFounders, even though we have asserted that totalFounders is 1.

Impact

1. Addresses which should not be rewarded as founders will continue to be rewarded.
2. The totalOwnership is not correctly adhered to, as there are "hidden founders" who accept a greater amount of ownership than explicitly intended.
3. The totalOwnership <= 99 invariant is defeated.

Code Snippet

/// @notice Update the list of allocation owners
/// @param newFounders the full list of founders
function updateFounders(IManager.FounderParams[] calldata newFounders) external onlyOwner {
    // Cache the number of founders
    uint256 numFounders = settings.numFounders;

    // Get a temporary array to hold all founders
    Founder[] memory cachedFounders = new Founder[](numFounders);

    // Cannot realistically overflow
    unchecked {
        // Add each founder to the array
        for (uint256 i; i < numFounders; ++i) {
            cachedFounders[i] = founder[i];
        }
    }

    // Keep a mapping of all the reserved token IDs we're set to clear.
    bool[] memory clearedTokenIds = new bool[](100);

    unchecked {
        // for each existing founder:
        for (uint256 i; i < cachedFounders.length; ++i) {
            // copy the founder into memory
            Founder memory cachedFounder = cachedFounders[i];

            // Delete the founder from the stored mapping
            delete founder[i];

            // Some DAOs were initialized with 0 percentage ownership.
            // This skips them to avoid a division by zero error.
            if (cachedFounder.ownershipPct == 0) {
                continue;
            }

            // using the ownership percentage, get reserved token percentages
            uint256 schedule = 100 / cachedFounder.ownershipPct;

            // 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;
            }
        }
    }

    // Clear values from storage before adding new founders
    settings.numFounders = 0;
    settings.totalOwnership = 0;
    emit FounderAllocationsCleared(newFounders);

    _addFounders(newFounders, reservedUntilTokenId);
}

Tool used

Visual Studio Code, Foundry

Recommendation

A naive solution may recommend to simply suggest to initialize baseTokenId correctly when calling updateFounders:

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

However, since this can be changed arbitrarily via calls to setReservedUntilTokenId, it is possible that the process of predicting tokens to unwrap can still be invalidated.

The most secure way to prevent this issue would be to use the modulo operator to our advantage, and merely wrap around the tokenRecipient mapping to zero all entries and prevent possible conflicts:

for (uint256 i = 0; i < 100;) {
    // Delete the founder from the stored mapping (if exists)
    delete founder[i];
    // Delete the preallocated token recipients (if exists)
    delete tokenRecipient[baseTokenId];

    unchecked { i++; }
}

In addition, it is recommended to create additional unit tests which experiment with non-zero _reservedUntilTokenId when making calls to setMockTokenParams.

Duplicate of #42