Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quorum votes have no effect for determining whether proposal is defeated or succeeded when token supply is low #607

Open
code423n4 opened this issue Sep 15, 2022 · 2 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

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.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Sep 15, 2022
code423n4 added a commit that referenced this issue Sep 15, 2022
@GalloDaSballo GalloDaSballo added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Sep 20, 2022
@GalloDaSballo
Copy link
Collaborator

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

@GalloDaSballo GalloDaSballo added duplicate This issue or pull request already exists and removed duplicate This issue or pull request already exists labels Sep 20, 2022
@tbtstl tbtstl added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Sep 26, 2022
@tbtstl
Copy link
Collaborator

tbtstl commented Sep 27, 2022

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.

@tbtstl tbtstl added sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons and removed sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants