Nouns Builder contest - Chom's results

Lines of code

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

Vulnerability details

Impact

Voter can vote without owning any NFT if they can borrow NFT on the creationBlock and hold it for 1 block then return it.

Voters can get too much voting power while using a little ETH (Borrowing fee), Then voters can spam voting NO or YES to the targeted proposals. If NO voting has been spammed, it may cause a valid proposal to be rejected.

Hackers can monitor mempool to know when the proposal is created and bribe a validator to attach NFT borrowing transactions into the same block as the proposal creation block.

Proof of Concept

proposal.timeCreated = uint32(block.timestamp);
...
weight = getVotes(_voter, proposal.timeCreated);
...
    function getVotes(address _account, uint256 _timestamp) public view returns (uint256) {
        return settings.token.getPastVotes(_account, _timestamp);
    }

proposal.timeCreated is set to the proposal creation timestamp which can be known by monitoring the mempool

weight is voting power at the proposal creation timestamp

Once hackers have seen the proposal creation transaction, they can sandwich the transaction with the NFT borrowing transaction. The voting power will get recorded and after 1 block, they return the borrowed NFT immediately. Once voting has started, the borrowed voting power remains there.

Recommended Mitigation Steps

Get voting power at proposal creation time - 1 second

weight = getVotes(_voter, proposal.timeCreated - 1);

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L361-L365 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L126-L129

Vulnerability details

Impact

If user A has votes == proposalThreshold and create a proposal, another user can cancel user A newly created proposal immediately.

Proof of Concept

Assume getVotes(userA, block.timestamp - 1) == proposal.proposalThreshold

function propose(
...
        unchecked {
            // Ensure the caller's voting weight is greater than or equal to the threshold
            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
        }
...

This will pass since getVotes(userA, block.timestamp - 1) == proposal.proposalThreshold (msg.sender == userA)

function cancel(
...
        // 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();
        }
...

Assume msg.sender == userB (another user)

Since getVotes(userA, block.timestamp - 1) == proposal.proposalThreshold (proposal.proposer == userA)

The check will be passed and the proposal will be cancelled.

Recommended Mitigation Steps

Use >= check instead of > in cancel

function cancel(
...
        // 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();
        }
...

Lines of code

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

Vulnerability details

Impact

Missing minimum and maximum votingDelay, voting period, and proposalThresholdBps checks.

Users may set unexpected parameters like proposalThresholdBps = 0 to bypass the minimum nft check. votingDelay = 0 to have voting start immediately. votingPeriod = 0 to have voting end immediately. votingDelay = 1000 years to prevent the new proposals from starting voting. votingPeriod = 1000 years to have voting pending forever.

votingDelay = 1000 years may be used to force veto against the community by disabling the community's ability to propose any new proposal.

Proof of Concept

In Nouns DAO, there are lower bound and upper bound checks of votingDelay, votingPeriod and proposalThresholdBps

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L61-L77

    /// @notice The minimum setable proposal threshold
    uint256 public constant MIN_PROPOSAL_THRESHOLD_BPS = 1; // 1 basis point or 0.01%

    /// @notice The maximum setable proposal threshold
    uint256 public constant MAX_PROPOSAL_THRESHOLD_BPS = 1_000; // 1,000 basis points or 10%

    /// @notice The minimum setable voting period
    uint256 public constant MIN_VOTING_PERIOD = 5_760; // About 24 hours

    /// @notice The max setable voting period
    uint256 public constant MAX_VOTING_PERIOD = 80_640; // About 2 weeks

    /// @notice The min setable voting delay
    uint256 public constant MIN_VOTING_DELAY = 1;

    /// @notice The max setable voting delay
    uint256 public constant MAX_VOTING_DELAY = 40_320; // About 1 week

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L137-L148

        require(
            votingPeriod_ >= MIN_VOTING_PERIOD && votingPeriod_ <= MAX_VOTING_PERIOD,
            'NounsDAO::initialize: invalid voting period'
        );
        require(
            votingDelay_ >= MIN_VOTING_DELAY && votingDelay_ <= MAX_VOTING_DELAY,
            'NounsDAO::initialize: invalid voting delay'
        );
        require(
            proposalThresholdBPS_ >= MIN_PROPOSAL_THRESHOLD_BPS && proposalThresholdBPS_ <= MAX_PROPOSAL_THRESHOLD_BPS,
            'NounsDAO::initialize: invalid proposal threshold bps'
        );

But in Zora, there aren't any checks on the lower bound and upper bound of these values

    /// @notice Updates the voting delay
    /// @param _newVotingDelay The new voting delay
    function updateVotingDelay(uint256 _newVotingDelay) external onlyOwner {
        emit VotingDelayUpdated(settings.votingDelay, _newVotingDelay);

        settings.votingDelay = SafeCast.toUint48(_newVotingDelay);
    }

    /// @notice Updates the voting period
    /// @param _newVotingPeriod The new voting period
    function updateVotingPeriod(uint256 _newVotingPeriod) external onlyOwner {
        emit VotingPeriodUpdated(settings.votingPeriod, _newVotingPeriod);

        settings.votingPeriod = SafeCast.toUint48(_newVotingPeriod);
    }

    /// @notice Updates the minimum proposal threshold
    /// @param _newProposalThresholdBps The new proposal threshold basis points
    function updateProposalThresholdBps(uint256 _newProposalThresholdBps) external onlyOwner {
        emit ProposalThresholdBpsUpdated(settings.proposalThresholdBps, _newProposalThresholdBps);

        settings.proposalThresholdBps = SafeCast.toUint16(_newProposalThresholdBps);
    }

    /// @notice Updates the minimum quorum threshold
    /// @param _newQuorumVotesBps The new quorum votes basis points
    function updateQuorumThresholdBps(uint256 _newQuorumVotesBps) external onlyOwner {
        emit QuorumVotesBpsUpdated(settings.quorumThresholdBps, _newQuorumVotesBps);

        settings.quorumThresholdBps = SafeCast.toUint16(_newQuorumVotesBps);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Add lower and upper bound checks of votingDelay, votingPeriod, and proposalThresholdBps to prevent users from entering unexpected values which may be used to force veto against the community by disabling the community's ability to propose any new proposal.

Lines of code

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

Vulnerability details

Impact

ERC721Votes: delegateBySig allows the user to vote to address 0, which causes the user to permanently lose his voting power. Voting power lost is not recoverable.

Proof of Concept

1. User calls delegateBySig function with valid signature and _to set as address(0)

    function delegateBySig(
        address _from,
        address _to,
        uint256 _deadline,
        uint8 _v,
        bytes32 _r,
        bytes32 _s
    ) external {
        // Ensure the signature has not expired
        if (block.timestamp > _deadline) revert EXPIRED_SIGNATURE();

        // Used to store the digest
        bytes32 digest;

        // Cannot realistically overflow
        unchecked {
            // Compute the hash of the domain seperator with the typed delegation data
            digest = keccak256(
                abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), keccak256(abi.encode(DELEGATION_TYPEHASH, _from, _to, nonces[_from]++, _deadline)))
            );
        }

        // Recover the message signer
        address recoveredAddress = ecrecover(digest, _v, _r, _s);

        // Ensure the recovered signer is the voter
        if (recoveredAddress == address(0) || recoveredAddress != _from) revert INVALID_SIGNATURE();

        // Update the delegate
        _delegate(_from, _to);
    }

2. Call _delegate(_from, address(0))
3. Update delegation[_from] = 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));
    }

4. Move voting power using _moveDelegateVotes(prevDelegate, _to, balanceOf(_from));
5. Since _to == address(0), so _moveDelegateVotes only subtracts the voting power of from address without adding voting power to anyone.

    /// @dev Transfers voting weight
    /// @param _from The address delegating votes from
    /// @param _to The address delegating votes to
    /// @param _amount The number of votes delegating
    function _moveDelegateVotes(
        address _from,
        address _to,
        uint256 _amount
    ) internal {
        unchecked {
            // If voting weight is being transferred:
            if (_from != _to && _amount > 0) {
                // If this isn't a token mint:
                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);
                }

                // If this isn't a token burn:
                if (_to != address(0)) {
                    // Get the recipients's number of checkpoints
                    uint256 nCheckpoints = numCheckpoints[_to]++;

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

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

                    // Update their voting weight
                    _writeCheckpoint(_to, nCheckpoints, prevTotalVotes, prevTotalVotes + _amount);
                }
            }
        }
    }

6. Voter lose his voting power forever

Recommended Mitigation Steps

Prevent delegateBySig to address(0)

    function delegateBySig(
        address _from,
        address _to,
        uint256 _deadline,
        uint8 _v,
        bytes32 _r,
        bytes32 _s
    ) external {
        if (_to == address(0)) revert INVALID_WALLET();

        // Ensure the signature has not expired
        if (block.timestamp > _deadline) revert EXPIRED_SIGNATURE();

Lines of code

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

Vulnerability details

Impact

Vetoer is a way to prevent unintended behavior from happening if a very rich attacker can gather at least 51% of NFTs. This can happen easily as normal NFT price is not high as Nouns DAO. The vetoer word may cause fud in the community, so some communities are likely to set vetoer to address(0) which is very dangerous.

Without vetoer, attackers can buy NFT from the market and create a proposal to transfer all funds in the treasury to themselves. In case the treasury fund is greater than 51% of NFT market cap.

Recommended Mitigation Steps

You must force everyone to have a vetoer in the first place to prevent this.

Check vetoer not equal to address(0) on initialization

    function initialize(
        address _treasury,
        address _token,
        address _vetoer,
        uint256 _votingDelay,
        uint256 _votingPeriod,
        uint256 _proposalThresholdBps,
        uint256 _quorumThresholdBps
    ) external initializer {
        // Ensure the caller is the contract manager
        if (msg.sender != address(manager)) revert ONLY_MANAGER();

        // Ensure non-zero addresses are provided
        if (_treasury == address(0)) revert ADDRESS_ZERO();
        if (_token == address(0)) revert ADDRESS_ZERO();
        if (_vetoer == address(0)) revert ADDRESS_ZERO();

        // Store the governor settings
        settings.treasury = Treasury(payable(_treasury));
        settings.token = Token(_token);
        settings.vetoer = _vetoer;
        settings.votingDelay = SafeCast.toUint48(_votingDelay);
        settings.votingPeriod = SafeCast.toUint48(_votingPeriod);
        settings.proposalThresholdBps = SafeCast.toUint16(_proposalThresholdBps);
        settings.quorumThresholdBps = SafeCast.toUint16(_quorumThresholdBps);

        // Initialize EIP-712 support
        __EIP712_init(string.concat(settings.token.symbol(), " GOV"), "1");

        // Grant ownership to the treasury
        __Ownable_init(_treasury);
    }

Lines of code

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

Vulnerability details

Impact

Precision is not enough for proposalThreshold and quorum. Collections with at least 20000 NFTs in total supply may have some trouble. These collections can't be set proposalThreshold = 1 or quorum = 1.

Proof of Concept

    /// @notice The current number of votes required to submit a proposal
    function proposalThreshold() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;
        }
    }

    /// @notice The current number of votes required to be in favor of a proposal in order to reach quorum
    function quorum() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;
        }
    }

If totalSupply = 20000, with the lowest settings (settings.proposalThresholdBps, settings.quorumThresholdBps = 1), it would returns (20000 * 1) / 10000 = 2. And it can't be lowered more.

Tools Used

Manual review

Recommended Mitigation Steps

Increase division to a more precise value such as 1e18 to allow high total supply NFT to always set threshold as 1

    /// @notice The current number of votes required to submit a proposal
    function proposalThreshold() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.proposalThresholdBps) / 1e18;
        }
    }

    /// @notice The current number of votes required to be in favor of a proposal in order to reach quorum
    function quorum() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.quorumThresholdBps) / 1e18;
        }
    }

Lines of code

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

Vulnerability details

Impact

Quorum and proposalThreshold may be dynamic if new tokens are minted / burned, attacker can mint NFT if possible, to increase the quorum to defeat the proposal in a malicious way.

An attacker may monitor mempool and mint token to increase proposalThreshold to prevent a proposal from being submitted.

Proof of Concept

    function proposalThreshold() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;
        }
    }

    /// @notice The current number of votes required to be in favor of a proposal in order to reach quorum
    function quorum() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;
        }
    }

proposalThreshold and quorum use totalSupply which is dynamic (can be increased once new NFT is minted)

Recommended Mitigation Steps

Use totalSupply snapshot of creation block - 1 (Do not use creation block as attacker may monitor mempool and know the proposal creation tx)

proposalThresholdBps and quorumThresholdBps are missing the maximum limit check

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

Since maximum proposalThreshold and quorum is (totalSupply * 10000) / 10000 = totalSupply, maximum would be 10000 (Not make sense to have more than totalSupply)

Should add the maximum limit check as follow

/// @notice Updates the minimum proposal threshold
/// @param _newProposalThresholdBps The new proposal threshold basis points
function updateProposalThresholdBps(uint256 _newProposalThresholdBps) external onlyOwner {
    require(_newProposalThresholdBps <= 10000, "Over Limit");
    emit ProposalThresholdBpsUpdated(settings.proposalThresholdBps, _newProposalThresholdBps);

    settings.proposalThresholdBps = SafeCast.toUint16(_newProposalThresholdBps);
}

/// @notice Updates the minimum quorum threshold
/// @param _newQuorumVotesBps The new quorum votes basis points
function updateQuorumThresholdBps(uint256 _newQuorumVotesBps) external onlyOwner {
    require(_newQuorumVotesBps <= 10000, "Over Limit");
    emit QuorumVotesBpsUpdated(settings.quorumThresholdBps, _newQuorumVotesBps);

    settings.quorumThresholdBps = SafeCast.toUint16(_newQuorumVotesBps);
}