code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L473-L477 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L116-L175 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L413-L456

Vulnerability details

Impact

At the early stage of the deployed DAO, it is possible that the following quorum function returns 0 because the token supply is low.

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

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

When calling the following propose function, proposal.quorumVotes = uint32(quorum()) is executed. If quorum() returns 0, proposal.quorumVotes is set to 0.

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

    function propose(
        address[] memory _targets,
        uint256[] memory _values,
        bytes[] memory _calldatas,
        string memory _description
    ) external returns (bytes32) {
        // Get the current proposal threshold
        uint256 currentProposalThreshold = proposalThreshold();

        // Cannot realistically underflow and `getVotes` would revert
        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();
        }

        // Cache the number of targets
        uint256 numTargets = _targets.length;

        // Ensure at least one target exists
        if (numTargets == 0) revert PROPOSAL_TARGET_MISSING();

        // Ensure the number of targets matches the number of values and calldata
        if (numTargets != _values.length) revert PROPOSAL_LENGTH_MISMATCH();
        if (numTargets != _calldatas.length) revert PROPOSAL_LENGTH_MISMATCH();

        // 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);

        // Used to store the snapshot and deadline
        uint256 snapshot;
        uint256 deadline;

        // Cannot realistically overflow
        unchecked {
            // Compute the snapshot and deadline
            snapshot = block.timestamp + settings.votingDelay;
            deadline = snapshot + settings.votingPeriod;
        }

        // Store the proposal data
        proposal.voteStart = uint32(snapshot);
        proposal.voteEnd = uint32(deadline);
        proposal.proposalThreshold = uint32(currentProposalThreshold);
        proposal.quorumVotes = uint32(quorum());
        proposal.proposer = msg.sender;
        proposal.timeCreated = uint32(block.timestamp);

        emit ProposalCreated(proposalId, _targets, _values, _calldatas, _description, descriptionHash, proposal);

        return proposalId;
    }

When determining the proposal's state, the following state function is called, which can execute else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) { return ProposalState.Defeated; }. If proposal.quorumVotes is 0, the proposal.forVotes < proposal.quorumVotes condition would always be false. Essentially, quorum votes have no effect at all for determining whether the proposal is defeated or succeeded when the token supply is low. Hence, critical proposals, such as for updating implementations or withdrawing funds from the treasury, that should not be passed if there are effective quorum votes for which the for votes fail to reach can be passed, or vice versa, so the impact can be huge.

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

    function state(bytes32 _proposalId) public view returns (ProposalState) {
        // Get a copy of the proposal
        Proposal memory proposal = proposals[_proposalId];

        // Ensure the proposal exists
        if (proposal.voteStart == 0) revert PROPOSAL_DOES_NOT_EXIST();

        // If the proposal was executed:
        if (proposal.executed) {
            return ProposalState.Executed;

            // Else if the proposal was canceled:
        } else if (proposal.canceled) {
            return ProposalState.Canceled;

            // Else if the proposal was vetoed:
        } else if (proposal.vetoed) {
            return ProposalState.Vetoed;

            // Else if voting has not started:
        } else if (block.timestamp < proposal.voteStart) {
            return ProposalState.Pending;

            // Else if voting has not ended:
        } else if (block.timestamp < proposal.voteEnd) {
            return ProposalState.Active;

            // Else if the proposal failed (outvoted OR didn't reach quorum):
        } else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) {
            return ProposalState.Defeated;

            // Else if the proposal has not been queued:
        } else if (settings.treasury.timestamp(_proposalId) == 0) {
            return ProposalState.Succeeded;

            // Else if the proposal can no longer be executed:
        } else if (settings.treasury.isExpired(_proposalId)) {
            return ProposalState.Expired;

            // Else the proposal is queued
        } else {
            return ProposalState.Queued;
        }
    }

Proof of Concept

Please append the following test in test\Gov.t.sol. This test will pass to demonstrate the described scenario.

    function test_QueueProposalWithZeroQuorumVotes() public {
        mintVoter1();

        (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = mockProposal();

        bytes32 descriptionHash = keccak256(bytes("test"));

        vm.warp(1 days);

        // token supply is only 4 at this moment
        assertEq(token.totalSupply(), 4);

        // the calculated quorum votes is 0 because the token supply is low
        assertEq((token.totalSupply() * governor.quorumThresholdBps()) / 10_000, 0);

        // voter1 creates the proposal
        vm.prank(voter1);
        governor.propose(targets, values, calldatas, "test");

        bytes32 proposalId = governor.hashProposal(targets, values, calldatas, descriptionHash);

        vm.warp(block.timestamp + governor.votingDelay());

        vm.prank(voter1);
        governor.castVote(proposalId, 1);

        vm.warp(block.timestamp + governor.votingPeriod());

        // quorum votes corresponding to the proposal is 0
        Proposal memory proposal = governor.getProposal(proposalId);
        assertEq(proposal.quorumVotes, 0);

        // the proposal is succeeded
        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Succeeded));

        // voter1 is able to queue the proposal
        vm.prank(voter1);
        governor.queue(proposalId);
        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Queued));
    }

Tools Used

VSCode

Recommended Mitigation Steps

A minimum quorum votes governance configuration that is at least 1 can be added. When quorum() returns 0 because the token supply is low, calling propose could set proposal.quorumVotes to the minimum quorum votes.

GalloDaSballo commented 1 year ago

Because rounding is determined by a mixture of totalSupply and quorumThresholdBps I believe the finding cannot be of high severity.

It is important to note that because totalSupply can be zero, especially if founders take no founder mint, the Governor contract may be griefed, for example by giving away allowances to setup for a future rug-pull.

Because the finding can cause a loss, and the code doesn't have specific ways to avoid that (e.g. minimum totalSupply) i believe Medium Severity to be appropriate

tbtstl commented 1 year ago

I think we're going to have to ACK this and move on – there's no clear minimum token requirement we can set at the beginning of a DAO lifecycle that couldn't be circumvented by the malicious user buying the first n tokens.

Situations like this will have to be handled by the DAOs vetoer until a quorum is deemed high enough.

code-423n4 / 2022-09-nouns-builder-findings