Nouns Builder contest - berndartmueller's results

Lines of code

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

Vulnerability details

Voting power for each NFT owner is persisted within timestamp-dependent checkpoints. Every voting power increase or decrease is recorded. However, the implementation of ERC721Votes creates separate checkpoints with the same timestamp for each interaction, even when the interactions happen in the same block/timestamp.

Impact

Checkpoints with the same timestamp will cause issues within the ERC721Votes.getPastVotes(..) function and will return incorrect votes for a given _timestamp.

Proof of Concept

lib/token/ERC721Votes.sol#L252-L253

/// @dev Records a checkpoint
/// @param _account The account address
/// @param _id The checkpoint id
/// @param _prevTotalVotes The account's previous voting weight
/// @param _newTotalVotes The account's new voting weight
function _writeCheckpoint(
    address _account,
    uint256 _id,
    uint256 _prevTotalVotes,
    uint256 _newTotalVotes
) private {
    // 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);
}

Consider the following example and the votes checkpoint snapshots:

Note: Bob owns a smart contract used to interact with the protocol

Transaction 0: Bob's smart contract receives 1 NFT through minting (1 NFT equals 1 vote)

Transaction 1: Bob's smart contract receives one more NFT through minting

Transaction 1: Within the same transaction 1, Bob's smart-contract delegates 2 votes to Alice

Transaction 1: Again within the same transaction 1, Bob's smart contract decides to reverse the delegation and self-delegates

Transaction 1: Bob's smart contract buys one more NFT

Bob now wants to vote (via his smart contract) on a governance proposal that has been created on timeCreated = 1 (timestamp 1).

Internally, the Governor._castVote function determines the voter's weight by calling getVotes(_voter, proposal.timeCreated).

governance/governor/Governor.sol#L275

weight = getVotes(_voter, proposal.timeCreated);

getVotes calls ERC721.getPastVotes internally:

governance/governor/Governor.sol#L462

function getVotes(address _account, uint256 _timestamp) public view returns (uint256) {
    return settings.token.getPastVotes(_account, _timestamp);
}

ERC721.getPastVotes(..., 1) tries to find the checkpoint within the while loop:

The middle checkpoint with index 2 matches the given timestamp 1 and returns 0 votes. This is incorrect, as Bob has 2 votes. Bob is not able to vote properly.

(Please be aware that this is just one of many examples of how this issue can lead to incorrect vote accounting. In other cases, NFT owners could have more voting power than they are entitled to)

Tools Used

Manual review

Recommended mitigation steps

Consider batching multiple checkpoints writes per block/timestamp similar to how NounsDAO records checkpoints.

Lines of code

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

Vulnerability details

Minting an NFT entitles the NFT owner to 1 vote. This is accomplished by using the ERC721Votes._afterTokenTransfer ERC721 hook. However, the ERC721Votes._afterTokenTransfer and ERC721Votes._delegate function are not using the correct (and self-delegated) delegators and delegatees (NFTs are self-delegated by default).

Impact

Self-delegating voting power following an NFT mint leaves the NFT owner with double the voting power.

Proof of Concept

The following test case demonstrates how an NFT owner can increase the voting power by self-delegating after a previous NFT mint.

Copy paste the following test case into Gov.t.sol and run the test via forge test --match-test test_doubleYourVotes:

function test_doubleYourVotes() public {
    mintVoter1();

    assertEq(token.balanceOf(voter1), 1);

    vm.warp(block.timestamp + 5 minutes);

    uint256 votesBeforeSelfDelegation = token.getPastVotes(voter1, block.timestamp - 1);

    assertEq(votesBeforeSelfDelegation, 1);

    vm.prank(voter1);
    token.delegate(voter1);

    vm.warp(block.timestamp + 5 minutes);

    uint256 votesAfterSelfDelegation = token.getPastVotes(voter1, block.timestamp - 1);

    assertEq(votesAfterSelfDelegation, 2);
}

Tools Used

Manual review

Recommended mitigation steps

Consider using delegates(..) within the ERC721Votes._afterTokenTransfer and ERC721Votes._delegate function:

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

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

function _delegate(address _from, address _to) internal {
    // Get the previous delegate
    address prevDelegate = delegates(_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));
}

Note: This requires changing the function visibility of ERC721Votes.delegates from external to public.

Attention! This recommended mitigation opens up a new issue. Delegating votes to the zero-address will break token transfers. Therefore it's advised to prevent vote delegation to address(0) via ERC721Votes.delegate and ERC721Votes.delegateBySig.

Lines of code

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

Vulnerability details

When creating a new governance proposal, the proposalId is generated by hashing the proposal data (_targets, _values, _calldatas, descriptionHash). To prevent duplicated proposals, the current Governor implementation checks if the proposalId exists already. If it exists, the call will revert with the PROPOSAL_EXISTS error.

Impact

Anyone can prevent others from creating governance proposals by front-running the create proposal transaction with the same data, followed by an immediate call to the Governor.cancel function.

This will prevent creating a proposal with the same proposal data. A proposal creator would have to slightly change the proposal to try to create it again (however, it can be prevented again due to the aforementioned issue)

Proof of Concept

governance/governor/Governor.propose

function propose(
    address[] memory _targets,
    uint256[] memory _values,
    bytes[] memory _calldatas,
    string memory _description
) external returns (bytes32) {
    [..]

    // Compute the description hash
    bytes32 descriptionHash = keccak256(bytes(_description));

    // Compute the proposal id
    bytes32 proposalId = hashProposal(_targets, _values, _calldatas, descriptionHash);

    // Get the pointer to store the proposal
    Proposal storage proposal = proposals[proposalId];

    // Ensure the proposal doesn't already exist
    if (proposal.voteStart != 0) revert PROPOSAL_EXISTS(proposalId); // @audit-info Reverts in case the proposals with the same data exists already

    [..]
}

governance/governor/Governor.cancel

Cancelling a proposal updates the proposal.canceled boolean property to true. proposal.voteStart is left unchanged (!= 0).

/// @notice Cancels a proposal
/// @param _proposalId The proposal id
function cancel(bytes32 _proposalId) external {
    // Ensure the proposal hasn't been executed
    if (state(_proposalId) == ProposalState.Executed) revert PROPOSAL_ALREADY_EXECUTED();

    // Get a copy of the proposal
    Proposal memory proposal = proposals[_proposalId];

    // Cannot realistically underflow and `getVotes` would revert
    unchecked {
        // Ensure the caller is the proposer or the proposer's voting weight has dropped below the proposal threshold
        if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold)
            revert INVALID_CANCEL();
    }

    // Update the proposal as canceled
    proposals[_proposalId].canceled = true;

    // If the proposal was queued:
    if (settings.treasury.isQueued(_proposalId)) {
        // Cancel the proposal
        settings.treasury.cancel(_proposalId);
    }

    emit ProposalCanceled(_proposalId);
}

Tools Used

Manual review

Recommended mitigation steps

Consider adding a per-account nonce storage variable (e.g. mapping(address => uint256) internal proposalCreatorNonces; to the Governor contract and include the proposalCreatorNonces[msg.sender]++ nonce within the computed proposal id.