Nouns DAO contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L143 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L219

Vulnerability details

Impact

Nouns NFTs represent governance voting power by having the NounsToken contract inherit the ERC721Checkpointable contract.

The ERC721Checkpointable contract is based on the Comp.sol implementation by Compound. One of the modifications is not needing to self-delegate. This is accomplished by returning the delegator's own address from the delegates(address delegator) function, in case there is no active delegation.

Delegations to the zero-address are prevented within the delegate(address delegatee) function by defaulting to the msg.sender if address(0) is passed as the delegatee parameter.

However, the delegateBySig(..) function does not validate the delegatee address, thus a Nouns NFT owner can delegate to the zero-address (for instance to purposefully "burn" voting power).

This will lock all Nouns NFTs from the owner and the owner loses the ability to transfer (e.g. sell) them.

Proof of Concept

Nouns NFT owner decides to "burn" votes by delegating to the zero-address by using delegateBySig(..).

base/ERC721Checkpointable.sol#L143

function delegateBySig(
    address delegatee,
    uint256 nonce,
    uint256 expiry,
    uint8 v,
    bytes32 r,
    bytes32 s
) public {
    bytes32 domainSeparator = keccak256(
        abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), getChainId(), address(this))
    );
    bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));
    bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash));
    address signatory = ecrecover(digest, v, r, s);
    require(signatory != address(0), 'ERC721Checkpointable::delegateBySig: invalid signature');
    require(nonce == nonces[signatory]++, 'ERC721Checkpointable::delegateBySig: invalid nonce');
    require(block.timestamp <= expiry, 'ERC721Checkpointable::delegateBySig: signature expired');
    return _delegate(signatory, delegatee); // @audit-info `delegatee` can potentially be address(0)
}

_delegate(..) will store address(0) as the current delegatee:

base/ERC721Checkpointable.sol#L202

 function _delegate(address delegator, address delegatee) internal {
    /// @notice differs from `_delegate()` in `Comp.sol` to use `delegates` override method to simulate auto-delegation
    address currentDelegate = delegates(delegator);

    _delegates[delegator] = delegatee;

    emit DelegateChanged(delegator, currentDelegate, delegatee);

    uint96 amount = votesToDelegate(delegator);

    _moveDelegates(currentDelegate, delegatee, amount);
}

_moveDelegates(..) subtracts the amount of owned Nouns NFTs from the current checkpoint votes and stores it. The current amount of votes is now 0.

Whenever the owner wants to transfer (for instance by selling on the open market) an NFT, _beforeTokenTransfer is called internally and _moveDelegates(..) is invoked.

base/ERC721Checkpointable.sol#L105

function _beforeTokenTransfer(
    address from,
    address to,
    uint256 tokenId
) internal override {
    super._beforeTokenTransfer(from, to, tokenId);

    /// @notice Differs from `_transferTokens()` to use `delegates` override method to simulate auto-delegation
    _moveDelegates(delegates(from), delegates(to), 1);
}

As delegates(from) returns the from address instead of the actual delegatee (which is address(0) in this case), _moveDelegates(..) reverts with an underflow error due to subtracting amount from 0 (see here)

base/ERC721Checkpointable.sol#L112-L115

function delegates(address delegator) public view returns (address) {
    address current = _delegates[delegator];
    return current == address(0) ? delegator : current;
}

base/ERC721Checkpointable.sol#L219

function _moveDelegates(
    address srcRep,
    address dstRep,
    uint96 amount
) internal {
    if (srcRep != dstRep && amount > 0) {
        if (srcRep != address(0)) {
            uint32 srcRepNum = numCheckpoints[srcRep];
            uint96 srcRepOld = srcRepNum > 0 ? checkpoints[srcRep][srcRepNum - 1].votes : 0;
            uint96 srcRepNew = sub96(srcRepOld, amount, 'ERC721Checkpointable::_moveDelegates: amount underflows');
            _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew);
        }

        if (dstRep != address(0)) {
            uint32 dstRepNum = numCheckpoints[dstRep];
            uint96 dstRepOld = dstRepNum > 0 ? checkpoints[dstRep][dstRepNum - 1].votes : 0;
            uint96 dstRepNew = add96(dstRepOld, amount, 'ERC721Checkpointable::_moveDelegates: amount overflows');
            _writeCheckpoint(dstRep, dstRepNum, dstRepOld, dstRepNew);
        }
    }
}

Here's a test to demonstrate the issue. Copy-paste the following code into nounsGovernance.test.ts and run the tests:

it.only("Owner delegating votes to address(0) will lock all Noun tokens from owner forever", async () => {
  await setTotalSupply(token, 3);

  // Give account0.address tokens
  await tokenCallFromDeployer.transferFrom(
    deployer.address,
    account0.address,
    0
  );

  // Vote is delegated to the token owner
  expect(await token.numCheckpoints(account0.address)).to.equal(1);
  expect(await token.delegates(account0.address)).to.equal(account0.address);
  expect(await token.getCurrentVotes(account0.address)).to.equal(1);

  /**
   * Delegate vote by signature to address(0)
   */
  const delegatee = constants.AddressZero;
  const nonce = 0;
  const expiry = 10e9;

  const signature = await account0._signTypedData(domain, Types, {
    delegatee,
    nonce,
    expiry,
  });
  const { v, r, s } = ethers.utils.splitSignature(signature);

  await (await token.delegateBySig(delegatee, nonce, expiry, v, r, s)).wait();

  // Still returns account0 as delegatee, even though the vote is "burned" to address(0) (as intended by the authors)
  expect(await token.delegates(account0.address)).to.equal(account0.address);

  /**
   * Transferring Nouns NFT token to someone else fails
   */
  await expect(
    tokenCallFromGuy.transferFrom(account0.address, account1.address, 0)
  ).to.be.revertedWith(
    "ERC721Checkpointable::_moveDelegates: amount underflows"
  );

  /**
   * Account 0 delegates vote back to self
   */
  await tokenCallFromGuy.delegate(account0.address);

  // Number of votes of account 0 is still 0
  expect(await token.getCurrentVotes(account0.address)).to.equal(0);

  /**
   * Transferring Nouns NFT token to someone else still fails
   */
  await expect(
    tokenCallFromGuy.transferFrom(account0.address, account1.address, 0)
  ).to.be.revertedWith(
    "ERC721Checkpointable::_moveDelegates: amount underflows"
  );
});

Tools Used

Manual review

Recommended mitigation steps

Consider preventing vote delegations to address(0) in ERC721Checkpointable.delegateBySig (see @audit-info annotation):

function delegateBySig(
    address delegatee,
    uint256 nonce,
    uint256 expiry,
    uint8 v,
    bytes32 r,
    bytes32 s
) public {
    require(delegatee != address(0), 'invalid delegatee'); // @audit-info add validation

    bytes32 domainSeparator = keccak256(
        abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), getChainId(), address(this))
    );
    bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));
    bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash));
    address signatory = ecrecover(digest, v, r, s);
    require(signatory != address(0), 'ERC721Checkpointable::delegateBySig: invalid signature');
    require(nonce == nonces[signatory]++, 'ERC721Checkpointable::delegateBySig: invalid nonce');
    require(block.timestamp <= expiry, 'ERC721Checkpointable::delegateBySig: signature expired');
    return _delegate(signatory, delegatee);
}

QA Report

• Low Risk
  • [L-01] Restrict ETH funds receivable by contract
  • [L-02] Proposals with certain states can be canceled

Low Risk

[L-01] Restrict ETH funds receivable by contract

Description

Native ETH fund transfers to contracts should be, if possible, limited to a limited set of addresses to prevent accidental native fund transfers from other sources.

Findings

NounsDAOLogicV2.sol#L1030

receive() external payable {}

Recommended mitigation steps

Modify the receive() function to only accept transfers from expected addresses.

[L-02] Proposals with certain states can be canceled

Description

Governance proposals which have not been executed so far could be canceled by the proposal proposer or by anyone if the proposer delegates dropped below the proposal threshold.

However, the current validation to prevent canceling executed proposals does not prevent canceling proposals with the following states:

• ProposalState.Defeated
• ProposalState.Expired
• ProposalState.Vetoed

Canceling proposals with one of these states will hide the "real" and previous state of the proposal and could lead to confusion.

Findings

NounsDAOLogicV2.sol#L347

function cancel(uint256 proposalId) external {
  require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal');

  [..]
}

Recommended mitigation steps

Consider preventing canceling proposals with the above-mentioned states.