Compromised or malicious vetoer can veto any proposals with unrestricted power

[code423n4]

Lines of code

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

Vulnerability details

Impact

The settings.vetoer, which is the first founder defined by the FounderParams, can call the following veto function to veto any proposals that are not yet executed, which immediately blocks these proposals from execution. Because the vetoer is just one founder, which can just be a single EOA, the chance of losing its private key and being compromised is not low. There is also no guarantee that the vetoer will not become malicious in the future. When the vetoer becomes compromised or malicious, all critical proposals, such as for updating implementations or withdrawing funds from the treasury, can be vetoed so the negative impact can be very high.

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

    function veto(bytes32 _proposalId) external {
        // Ensure the caller is the vetoer
        if (msg.sender != settings.vetoer) revert ONLY_VETOER();

        // Ensure the proposal has not been executed
        if (state(_proposalId) == ProposalState.Executed) revert PROPOSAL_ALREADY_EXECUTED();

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

        // Update the proposal as vetoed
        proposal.vetoed = true;

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

        emit ProposalVetoed(_proposalId);
    }

Proof of Concept

Please append the following tests in test\Gov.t.sol. These tests will pass to demonstrate the vetoer's power for vetoing pending, active, and queued proposals.

    function test_VetoPendingProposal() public {
        bytes32 proposalId = createProposal();

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Pending));

        // vetoer can veto a pending proposal without a delay
        vm.prank(founder);
        governor.veto(proposalId);
        assertEq(uint8(governor.state(proposalId)), uint8(ProposalState.Vetoed));
    }

    function test_VetoActiveProposal() public {
        mintVoter1();

        bytes32 proposalId = createProposal();

        uint256 votingDelay = governor.votingDelay();
        vm.warp(block.timestamp + votingDelay + 1);

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Active));

        // vetoer can veto an active proposal without a delay
        vm.prank(founder);
        governor.veto(proposalId);
        assertEq(uint8(governor.state(proposalId)), uint8(ProposalState.Vetoed));
    }

    function test_VetoQueuedProposal() public {
        mintVoter1();

        bytes32 proposalId = createProposal();

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

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Active));

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

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

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Succeeded));

        vm.prank(voter1);
        governor.queue(proposalId);

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Queued));

        // vetoer can veto a queued proposal without a delay
        vm.prank(founder);
        governor.veto(proposalId);
        assertEq(uint8(governor.state(proposalId)), uint8(ProposalState.Vetoed));
    }

Tools Used

VSCode

Recommended Mitigation Steps

A token supply threshold governance configuration can be added. Before the token supply exceeds this threshold, the vetoer can remain in full power for protecting against the 51% attack on the deployed DAO while the token supply is low.

After the token supply exceeds this threshold, the following changes can be considered for restricting the vetoer's power.

1. The vetoer is only allowed to veto a proposal during a defined period after the voting is done. The proposal is not allowed to be executed during this period.
2. The vetoer is only allowed to veto the passed proposal when the number of support and against votes are very close or the number of support votes is much higher than the against votes.

[tbtstl]

The intention of the vetoer is to ensure they can veto. This is by design.

[GalloDaSballo]

I believe that in the spirit of transparency, because the 51% without Vetoer is a valid attack, then the idea the Vetoer can be the weak link also has to be entertained.

While we would all agree that there are ways to mitigate this, such as using a fairly strong multisig with members of other projects, we would also agree concede that the Vetoer would still be the last piece before decentralization.

In that sense, an early project does carry further risk because of the Vetoer, and while a Vetoer itself is not inherently good nor bad, the Vetoer itself could create opportunities for sophisticated attacks.

Other reports spoke about bribing voters, or pure 51% attacks, and all of these are contingent on the Vetoer being unable to react / defend the protocol at the right time.

So, if we agree that the lack of Vetoer is a vulnerability we must also agree that the presence of a tyrannical Vetoer is a vulnerability as well.

In that sense we must admit that Governance itself can create further problems and risks and perhaps issuing enough tokens to allow the burning of the Vetoer is the first step towards a more decentralized Project.

[GalloDaSballo]

Because CodeArena is a user facing Project, where our reports can be used to persuade end-users that the protocols are safe, per the logic detailed above, the finding is valid and of Medium Severity per our rules.