Nouns Builder contest - 0x52's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L242-L256

Vulnerability details

Impact

Votes can be multiplied allowing for governance attacks

Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L242-L256

// Get the pointer to store the checkpoint
Checkpoint storage checkpoint = checkpoints[_account][_id];

// Record the updated voting weight and current time
checkpoint.votes = uint192(_newTotalVotes);
checkpoint.timestamp = uint64(block.timestamp);

emit DelegateVotesChanged(_account, _prevTotalVotes, _newTotalVotes);

After each token transfer, ERC721Votes.sol#_moveDelegateVote is called, which calls ERC721Votes.sol#_writeCheckpoint (shown above). If called multiple times in one block, it will write multiple checkpoints with the same timestamp as there is no logic to prevent it. Allowing multiple checkpoints per block can be abused using a quirk with how binary search in getPastVotes works. By carefully transferring between accounts and controlling account array lengths getPastVotes can be manipulated to return a number of votes higher than the total number of NFTs owned. First let's break down how getPastVotes selects which checkpoint to return.

if (accountCheckpoints[lastCheckpoint].timestamp <= _timestamp) return accountCheckpoints[lastCheckpoint].votes;

When the timestamp <= lastCheckpoint.timestamp getPastVotes immediately short circuits and returns the most recent checkpoint.

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L85-L114

uint256 high = lastCheckpoint;
uint256 low;
uint256 middle;

// Used to temporarily hold a checkpoint
Checkpoint memory cp;

while (high > low) {
    // Find the id of the middle checkpoint
    middle = high - (high - low) / 2;

    // Get the middle checkpoint
    cp = accountCheckpoints[middle];

    // If the timestamp is a match:
    if (cp.timestamp == _timestamp) {
        // Return the voting weight
        return cp.votes;

        // Else if the timestamp is before the one looking for:
    } else if (cp.timestamp < _timestamp) {
        // Update the lower bound
        low = middle;

        // Else update the upper bound
    } else {
        high = middle - 1;
    }
}

Now if we look at the binary search portion of the code, if middle.timestamp == timestamp, it immediately returns the middle point. With the above code we figure what checkpoint would be the first to return when lastCheckpoint = 4 (array length of 5)

middle = high - (high - low) / 2 
middle = 4 - (4 - 0) / 2 = 2

In the case where an array is 5 in length, the first checkpoint that will be returned is accountCheckpoints[2]. If the accountCheckpoints[2].timestamp == timestamp then the accountCheckpoints[2].votes will be returned without any further iterations. Next we'll walk through a setup in which we can take 3 NFTs and generate a total of 4 votes across 2 accounts. Accounts A and B are the accounts of concern and X is the account the originally had the 3 NFTs.

Transactions grouped by blocks:

Block 1
1. Transfer X -> A; A in 1
2. Transfer X -> A; A in 1
3. Transfer X -> A; A in 1

Block 2
1. Transfer A -> B; A out 1; B in 1
2. Transfer A -> B; A out 1; B in 1
3. Transfer A -> B; A out 1; B in 1
4. Transfer B -> A; B out 1; A in 1

Block 3
1. Transfer B -> X; B out 1

Resulting arrays from transfers shown above:

Array A
[0] Block1.timestamp, 1
[1] Block1.timestamp, 2
[2] Block1.timestamp, 3
[3] Block2.timestamp, 2
[4] Block2.timestamp, 1
[5] Block2.timestamp, 0
[6] [Block2.timestamp, 1] - returned

Array B
[0] Block2.timestamp, 1
[1] Block2.timestamp, 2
[2] [Block2.timestamp, 3] - returned
[3] Block2.timestamp, 2
[4] Block3.timestamp, 1

TotalVotes = 3 + 1 = 4

When getPastVotes is called on account A with Block2.timestamp, the timestamp matches the most recent checkpoint and therefore returns 1. When called on account B it will return 3 (B[2].votes) because that will be the first checkpoint selected by the binary search and the timestamps match. This means that between account A and B, the total number of votes returned is 4, even though the user only owns 3.

The attack shown above is just the simplest version of this attack vector. It can be layered with more NFTs and accounts to increase votes to a much larger extent and with much higher gas efficiency.

This increase in votes could be abused by creating the proposal in the same block as manipulating the votes. That way when Governor.sol calls getPastVotes, the timestamps will match exactly, returning the manipulated vote count. This attack surface area is reduced by the vetoer in Governor.sol but that is not a suitable mitigation because the vetoer may not be active for any number of reasons, including the role being burned by governance.

Tools Used

Recommended Mitigation Steps

The easiest way to mitigate this is to change ERC721Votes.sol#_writeCheckpoint so that it only allows a single checkpoint per block:

function _writeCheckpoint(
    address _account,
    uint32 _id,
    uint96 _prevTotalVotes,
    uint96 _newTotalVotes
) private {

    // Check is current block.timestamp matches previous checkpoint timestamp
    // Subtracting 1 from _id because _id is incremented by _moveDelegateVotes before calling this function
    if (_id > 0 && checkpoints[_account][_id - 1].timestamp == block.timestamp) {
        checkpoints[_account][_id - 1].votes = _newTotalVotes;

        // Remove checkpoint added by _moveDelegateVotes
        numCheckpoints[_account] = _id - 1;
    } else {
        // Get the pointer to store new checkpoint
        Checkpoint storage checkpoint = checkpoints[_account][_id];

        // Record the updated voting weight and current time
        checkpoint.votes = uint192(_newTotalVotes);
        checkpoint.timestamp = uint64(block.timestamp);
    }

    emit DelegateVotesChanged(_account, _prevTotalVotes, _newTotalVotes);
}

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L171-L202

Vulnerability details

Impact

Minting is bricked by having too many properties

Proof of Concept

tokenAttributes[i + 1] = uint16(seed % numItems);

If properties.length is ever greater than 14 the whole minting process will be broken because onMinted will revert when token is minted to auction.

Tools Used

Recommended Mitigation Steps

Add safeguard to MetadataRenderer.sol#addProperties to revert if number of properties >= 15

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L207-L213

Vulnerability details

Impact

Inaccurate quorum count and potential loss of treasury control

Proof of Concept

Each time an auction is started, a token is minted to Auction.sol. Each time a token is minted it increments settings.totalSupply. If an auction finishes and there are no bids, the token is burned. The issue is that in Token.sol#burn settings.totalSupply is not decremented. This means that each time an auction closes without a bid, settings.totalSupply increases but the voting supply of the token doesn't. This can lead to potential quorum issues because the threshold in Governor.sol is calculate using Token.sol#settings.totalSupply. Each auction that passes without a bid further skews the quorum threshold. Eventually if enough auctions are missed, settings.tokenSupply could be so skewed that it would be impossible to get any proposal above quorum, causing loss of access to treasury funds.

Tools Used

Recommended Mitigation Steps

Rewrite Token.sol#burn, adding a line to decrement settings.totalSupply:

function burn(uint256 _tokenId) external {
    // Ensure the caller is the auction house
    if (msg.sender != settings.auction) revert ONLY_AUCTION();

    settings.totalSupply--;

    // Burn the token
    _burn(_tokenId);
}