Nouns Builder contest - PwnPatrol's results

Lines of code

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

Vulnerability details

Impact

When tokens are transferred from one user to another, votes should be moved from the delegatee of the sender to the delegatee of the receiver. Instead, they are transferred from the sender to the receiver. Because the _moveDelegateVotes() function is unchecked, this can cause an underflow to give the user 2 ** 256 - 1 votes.

Proof of Concept

Transfers of the NFT call the _afterTokenTransfer() function, which calls _moveDelegateVotes() to transfer one vote from the sender to the receiver. However, the vote should be transferred from the sender's delegate to the receiver's delegate.

function _afterTokenTransfer(
    address _from,
    address _to,
    uint256 _tokenId
) internal override {
    // Transfer 1 vote from the sender to the recipient
    _moveDelegateVotes(_from, _to, 1);

    super._afterTokenTransfer(_from, _to, _tokenId);
}

The _moveDelegateVotes() function performs the following function to adjust the number of votes:

if (_from != address(0)) {
    // Get the sender's number of checkpoints
    uint256 nCheckpoints = numCheckpoints[_from]++;

    // Used to store the sender's previous voting weight
    uint256 prevTotalVotes;

    // If this isn't the sender's first checkpoint: Get their previous voting weight
    if (nCheckpoints != 0) prevTotalVotes = checkpoints[_from][nCheckpoints - 1].votes;

    // Update their voting weight
    _writeCheckpoint(_from, nCheckpoints, prevTotalVotes, prevTotalVotes - _amount);
}

This block of code is wrapped in an unchecked block. This means that if a delegated user whose previous total votes is 0 transfers away a token, the checkpoint will be written with prevTotalVotes - _amount = 0 - 1 as the new value.

This will underflow, leading to 2 ** 256 - 1 votes for the sender.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Change the _afterTokenTransfer() function to transfer votes from the delegatee of the sender to the delegatee of the receiver:

_moveDelegateVotes(delegates(_from), delegates(_to), 1);

Lines of code

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

Vulnerability details

Impact

Because multiple checkpoints can be made within the same block, users can move tokens around and trigger multiple checkpoints with different balances, then architect the later binary search to hit the higher balances, gaming the voting system to give themselves more votes.

Proof of Concept

Checkpoints within the same block should overwrite each other, as they do in other forks of Compound Governor Bravo (including Nouns). Instead, they are appended in the array.

This can lead to consecutive checkpoints having the same block.timestamp.

Because of this and the fact that getPastVotes() function makes a binary search, this function can read the wrong checkpoint, which enables a possible sybil attack with 1 NFT. An attacker can send an NFT to many of his accounts within the same block in a way that can influence the binary search so that it selects malicious checkpoints.

function getPastVotes(address _account, uint256 _timestamp) public view returns (uint256) {
        [...]

        // Get the account's number of checkpoints
        uint256 nCheckpoints = numCheckpoints[_account];

        [...]

        unchecked {
            // Get the latest checkpoint id
            // Cannot underflow as `nCheckpoints` is ensured to be greater than 0
            uint256 lastCheckpoint = nCheckpoints - 1;

            [...]

            // Otherwise, find a checkpoint with a valid timestamp
            // Use the latest id as the initial upper bound
            uint256 high = lastCheckpoint;
            uint256 low;
            uint256 middle;

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

            // While a valid checkpoint is to be found:
            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;
                }
            }

            return accountCheckpoints[low].votes;
        }
    }

Recommended Mitigation Steps

If multiple checkpoints are created in the same block, they should be overwritten (the way there are in the live Nouns token):

if (nCheckpoints > 0 && checkpoints[delegatee][nCheckpoints - 1].fromBlock == blockNumber) {
      checkpoints[delegatee][nCheckpoints - 1].votes = newVotes;
} else {
      checkpoints[delegatee][nCheckpoints] = Checkpoint(blockNumber, newVotes);
      numCheckpoints[delegatee] = nCheckpoints + 1;
}

Lines of code

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

Vulnerability details

Impact

When a user has undelegated tokens and delegates them to another user, the owner retains all votes for their tokens, while the delegatee also gains those votes.

By chaining this attack together, this could allow a user to generate unlimited votes, taking over the system and passing anything through governance.

Proof of Concept

When votes are delegated, the _delegate() function starts by finding the previous delegate with delegation[from]. If the votes aren't delegated, this returns the zero address.

When _moveDelegateVotes() is called, prevDelegate is passed as an argument, which passes address(0) instead of the delegator's address.

The function has a check if (_from != address(0)) before writing the checkpoint, so the result is that no votes are removed or checkpoint is written for the delegator, while the delegatee receives their delegated votes.

This is shown by the following test:

function test_DelegationDoesNotDecrease() public {
    console.log("Number of Tokens: ", token.balanceOf(otherUsers[0]));

    console.log("Initial Votes: ", token.getVotes(otherUsers[0]));
    console.log("Friend Initial Votes: ", token.getVotes(otherUsers[1]));

    console.log("Delegating...");
    vm.prank(otherUsers[0]);
    token.delegate(otherUsers[1]);

    console.log("After Votes: ", token.getVotes(otherUsers[0]));
    console.log("Friend After Votes: ", token.getVotes(otherUsers[1]));
}

This vulnerability can be exploited to create unlimited votes. If a user holds their token in Address A, they can delegate their votes to Address Z, transfer to Address B, redelegate to Address Z, etc until Address Z has accumulated an arbitrary number of votes. The result is that governance can be completely taken over by this attack.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Rather than being set as delegation[from], prevDelegate should be set using the delegates() function, which returns the user's own address if their tokens aren't delegated.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L248-L254

Vulnerability details

Impact

If the first auction is paused and unpaused in a protocol deployed with no founder fees, the highest bid (as well as the first NFT), will get stuck in the protocol with no ability to retrieve either of them.

Proof of Concept

In a protocol with founder ownership percentage set to 0, the first tokenId put to auction is #0.

If the first auction in such a protocol is paused and unpaused, the check for if (auction.tokenId == 0) will pass and _createAuction() will automatically be called, minting the next token and starting a new auction based on token #1.

The result is that highestBid and highestBidder are reset, the first auction is never settled, and the highest bid (as well as NFT #0) will remain stuck in the platform.

The following test confirms this finding:

function test_PauseAndUnpauseInFirstAuction() public {
    address bidder1 = vm.addr(0xB1);
    address bidder2 = vm.addr(0xB2);

    vm.deal(bidder1, 100 ether);
    vm.deal(bidder2, 100 ether);

    console.log("Deploying with no founder pct...");
    deployMockWithEmptyFounders();

    console.log("Unpausing...");
    vm.prank(founder);
    auction.unpause();

    console.log("Bidder makes initial bid.");
    vm.prank(bidder1);
    auction.createBid{ value: 1 ether }(0);
    (uint256 tokenId_, uint256 highestBid_, address highestBidder_,,,) = auction.auction();
    console.log("Currently bidding for ID ", tokenId_);
    console.log("Highest Bid: ", highestBid_, ". Bidder: ", highestBidder_);
    console.log("Contract Balance: ", address(auction).balance);
    console.log("--------");

    console.log("Pausing and unpausing auction house...");
    vm.startPrank(address(treasury));
    auction.pause();
    auction.unpause();
    vm.stopPrank();

    console.log("Bidder makes new bid.");
    vm.prank(bidder2);
    auction.createBid{ value: 0.5 ether }(1);
    (uint256 tokenId2_, uint256 highestBid2_, address highestBidder2_,,,) = auction.auction();
    console.log("Currently bidding for ID ", tokenId2_);
    console.log("Highest Bid: ", highestBid2_, ". Bidder: ", highestBidder2_);
    console.log("Contract Balance: ", address(auction).balance);

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Remove the block in unpause() that transfers ownership and creates an auction if auction.tokenId == 0 and trigger those actions manually in the deployment flow.

Lines of code

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

Vulnerability details

Impact

In projects where Founders are allocated a large share of the tokens, the final Founder in the array frequently doesn't receive their fair share of the tokens due to a bug in the _addFounders() function.

Proof of Concept

All Founder tokens are allocated based on the tokenRecipient mapping, which maps a 2 digit number ("baseTokenId") to a founder, and gives them all tokens that end with that 2 digit number.

This mapping is filled in through the _addFounders() function, which iterates through each founder and, for each of them:

• Calculates a "schedule", the frequency at which they should get a token (100 / percentage)
• Runs a loop "percentage" times, counting forward by "schedule", finding the next token that hasn't been claimed by another founder, and setting them as the recipient of that token in tokenRecipients
• At the end of each loop, calls (baseTokenId += schedule) % 100; to increment the baseTokenId modulo 100

However, the code to increment baseTokenId does not actually increment modulo 100, so baseTokenId continues to iterate up above 100.

The result is that, for Founders later in the array, if their schedules overlap with the earlier Founders, their baseTokenIds will be pushed over 100, effectively making them useless and resulting in an underallocation of tokens.

The following test shows a situation where 3 Founders should receive 79 tokens, but instead end up receiving only 72:

function test_FounderSharesWorkCorrectly() public {
        createUsers(3, 1 ether);

        // uint f1Per, uint f2Per, uint f3Per
        uint f1Per = 34;
        uint f2Per = 34;
        uint f3Per = 11;

        address[] memory wallets = new address[](3);
        uint256[] memory percents = new uint256[](3);
        uint256[] memory vestExpirys = new uint256[](3);

        percents[0] = f1Per;
        percents[1] = f2Per;
        percents[2] = f3Per;
        uint256 end = 4 weeks;

        unchecked {
            for (uint256 i; i < 3; ++i) {
                wallets[i] = otherUsers[i];
                vestExpirys[i] = end;
            }
        }

        deployWithCustomFounders(wallets, percents, vestExpirys);

        assertEq(token.totalFounders(), 3);
        assertEq(token.totalFounderOwnership(), f1Per + f2Per + f3Per);

        uint sumOfFounderTokens;

        for (uint256 i; i < 100; ++i) {
            address rec = token.getScheduledRecipient(i).wallet;
            if (rec != address(0)) {
                sumOfFounderTokens++;
            }
        }
        console.log("Sum of Founder Tokens: ", sumOfFounderTokens);
        console.log("Proper Sum of Founder Tokens: ", f1Per + f2Per + f3Per);
        assert(sumOfFounderTokens == f1Per + f2Per + f3Per);

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Fix the calculation to increment baseTokenId to return the value modulo 100:

baseTokenId = (baseTokenId + schedule) % 100;

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L128 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L363

Vulnerability details

Impact

The number of votes needed to create a proposal is 1 less than the number of votes you require to stop another user from cancelling your proposal. This is clearly an unintentional mismatch, and could lead to users creating proposals that are immediately cancelled by other users.

Proof of Concept

When creating a proposal, the following code checks whether you have sufficient votes:

if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();

This code only reverts if you have less votes than proposalThreshold, meaning that if you have proposalThreshold or more votes, the proposal is created.

However, if another user wants to cancel your proposal, this is the code that checks:

if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold) revert INVALID_CANCEL();

This code only reverts if you have more votes than proposalThreshold, meaning that if you have proposalThreshold or less votes, the proposal is cancelled.

This creates an inequality, where if you have exactly proposalThreshold votes, you are able to create a proposal, but another user is also able to immediately cancel it, which clearly isn't the intended behavior of the system.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the check in cancel() to revert if the proposal creator has greater than or equal to the proposalThreshold, so that the equality holds:

if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) >= proposal.proposalThreshold)
        revert INVALID_CANCEL();

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L179-L190

Vulnerability details

Impact

ERC721Votes allows users to delegate to address 0. Both directly and by sig.

Proof of Concept

Users can accidentally delegate to address 0.

    function _delegate(address _from, address _to) internal {
        // Get the previous delegate
        address prevDelegate = delegation[_from];

        // Store the new delegate
        delegation[_from] = _to;

        emit DelegateChanged(_from, prevDelegate, _to);

        // Transfer voting weight from the previous delegate to the new delegate
        _moveDelegateVotes(prevDelegate, _to, balanceOf(_from));
    }

when this happens, votes are lost because _moveDelegateVotes() function doesn't move votes to address 0.

This is also true for delegating by signature.

Recommended Mitigation Steps

Revert on address 0 or delegate to self

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L596-L602

Vulnerability details

Impact

Loss of Veto Power can Lead to 51% Attack

Proof of Concept

The veto power is import functionality for current NounsDAO in order to protect their treasury from malicious proposals. However, there is a lack of 2-step transfer for vetoer role. This might lead to Nounders losing their veto power unintentionally and open to 51% attack which can drain their entire treasury.

Recommended Mitigation Steps

We recommend implementing a 2-step transfer for the vetoer role

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L80-L125

Vulnerability details

Impact

1 founder can silently steal all NFTs from users and other founders

Proof of Concept

If the first founder has founderPct = 257 then he will get all NFT allocations. The function will not revert because founderPct will overflow uint8, thus bypassing the safety checks. What is more, all the other founders will get their fair share allocations but with baseTokenId > 100 thus effectively they will not get any NFT (because allocations are modulo % 100).

// For each founder:
for (uint256 i; i < numFounders; ++i) {
    // Cache the percent ownership
    uint256 founderPct = _founders[i].ownershipPct;

    // Continue if no ownership is specified
    if (founderPct == 0) continue;

    // Update the total ownership and ensure it's valid
    if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

    // Compute the founder's id
    uint256 founderId = settings.numFounders++;

    // Get the pointer to store the founder
    Founder storage newFounder = founder[founderId];

    // Store the founder's vesting details
    newFounder.wallet = _founders[i].wallet;
    newFounder.vestExpiry = uint32(_founders[i].vestExpiry);
    newFounder.ownershipPct = uint8(founderPct);

    // Compute the vesting schedule
    uint256 schedule = 100 / founderPct;

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

    // 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 += schedule) % 100;
    }
}

This is bad for both users and other founders and can be not noticed until damage is done that's why we escalate it to High.

Recommended Mitigation Steps

We recommend converting and storing foundersPct at the top of the for loop.

for (uint256 i; i < numFounders; ++i) {
    uint256 founderPct = _founders[i].ownershipPct;
    uint8 founderPct = uint8(founderPct);
    [...]
}

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L354

Vulnerability details

Impact

Previous highest bidder can scare away and grief current bidder with unexpected tx cost

Proof of Concept

In createBid() the ETH refund to previous highest bidder is sent in a raw call with 50k gas stipend. It's quite a lot and can be all drained by malicious Bidder in a simple for loop

uint i;
fallback() pyabale external {
    while(true) { i++; }
}

Assuming gas cost ~15gwei and ETH price 4k USD. 50k gas * 15 gwei per gas = 0.0075 ETH which would be 30$.

This can successfully scare away the next bidder or make him pay unecessary cost

Recommended Mitigation Steps

Switch to a pull-payment instead of push-payment or reduce gas stipend to, say, 3k