Admin can set params.quorumCoefficient to be arbitrary high, blocking the majority of proposals

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L726-L736 https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L771-L776

Vulnerability details

There is no restrictions for how big quorumCoefficient can be. I.e. it now can be set to, say 1e18*1e6, yielding 1e18x multiplier for against votes, meaning that of there is any against vote of any size, the required quorum becomes params.maxQuorumVotesBPS, i.e. up to MAX_QUORUM_VOTES_BPS_UPPER_BOUND or 60% of the total supply, which will become more and more restrictive along with future emissions.

This way admin can effectively block almost all proposals as the required quorum will render them as Defeated. This will de facto disable the key functionality of the protocol. Without the ability to implement proposals the protocol tokens become less valuable, which leads to monetary loss for all the token holders.

Proof of Concept

admin can use _setQuorumCoefficient() and _setDynamicQuorumParams() to set any params.quorumCoefficient, it's not restricted anyhow:

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L726-L736

    function _setQuorumCoefficient(uint32 newQuorumCoefficient) external {
        require(msg.sender == admin, 'NounsDAO::_setQuorumCoefficient: admin only');
        DynamicQuorumParams memory params = getDynamicQuorumParamsAt(block.number);

        uint32 oldQuorumCoefficient = params.quorumCoefficient;
        params.quorumCoefficient = newQuorumCoefficient;

        _writeQuorumParamsCheckpoint(params);

        emit QuorumCoefficientSet(oldQuorumCoefficient, newQuorumCoefficient);
    }

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L771-L776

        DynamicQuorumParams memory params = DynamicQuorumParams({
            minQuorumVotesBPS: newMinQuorumVotesBPS,
            maxQuorumVotesBPS: newMaxQuorumVotesBPS,
            quorumCoefficient: newQuorumCoefficient
        });
        _writeQuorumParamsCheckpoint(params);

Anyone with at least minimal voting power can vote against the proposal and, given bloated params.quorumCoefficient it doesn't matter how big that against vote was, the quorum required can be put to be params.maxQuorumVotesBPS, i.e. up to MAX_QUORUM_VOTES_BPS_UPPER_BOUND or 60%:

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L903-L913

    function dynamicQuorumVotes(
        uint256 againstVotes,
        uint256 totalSupply,
        DynamicQuorumParams memory params
    ) public pure returns (uint256) {
        uint256 againstVotesBPS = (10000 * againstVotes) / totalSupply;
        uint256 quorumAdjustmentBPS = (params.quorumCoefficient * againstVotesBPS) / 1e6;
        uint256 adjustedQuorumBPS = params.minQuorumVotesBPS + quorumAdjustmentBPS;
        uint256 quorumBPS = min(params.maxQuorumVotesBPS, adjustedQuorumBPS);
        return bps2Uint(quorumBPS, totalSupply);
    }

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L85-L86

    /// @notice The upper bound of maximum quorum votes basis points
    uint256 public constant MAX_QUORUM_VOTES_BPS_UPPER_BOUND = 6_000; // 4,000 basis points or 60%

This can put even overly sucessful proposal to become ProposalState.Defeated:

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L877-L889

    function quorumVotes(uint256 proposalId) public view returns (uint256) {
        Proposal storage proposal = _proposals[proposalId];
        if (proposal.totalSupply == 0) {
            return proposal.quorumVotes;
        }

        return
            dynamicQuorumVotes(
                proposal.againstVotes,
                proposal.totalSupply,
                getDynamicQuorumParamsAt(proposal.creationBlock)
            );
    }

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L441-L444

        } else if (block.number <= proposal.endBlock) {
            return ProposalState.Active;
        } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < quorumVotes(proposal.id)) {
            return ProposalState.Defeated;

In order to be queued a proposal have to be in state(proposalId) == ProposalState.Succeeded:

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L285-L303

    function queue(uint256 proposalId) external {
        require(
            state(proposalId) == ProposalState.Succeeded,
            'NounsDAO::queue: proposal can only be queued if it is succeeded'
        );
        Proposal storage proposal = _proposals[proposalId];
        uint256 eta = block.timestamp + timelock.delay();
        for (uint256 i = 0; i < proposal.targets.length; i++) {
            queueOrRevertInternal(
                proposal.targets[i],
                proposal.values[i],
                proposal.signatures[i],
                proposal.calldatas[i],
                eta
            );
        }
        proposal.eta = eta;
        emit ProposalQueued(proposalId, eta);
    }

Recommended Mitigation Steps

Consider putting an upper bound on params.maxQuorumVotesBPS:

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L726-L736

    function _setQuorumCoefficient(uint32 newQuorumCoefficient) external {
        require(msg.sender == admin, 'NounsDAO::_setQuorumCoefficient: admin only');
        DynamicQuorumParams memory params = getDynamicQuorumParamsAt(block.number);
+       require(newQuorumCoefficient < MAX_QUORUM_COEFFICIENT,
+           'NounsDAO::_setQuorumCoefficient: new coefficient is too high'
+       );

        uint32 oldQuorumCoefficient = params.quorumCoefficient;
        params.quorumCoefficient = newQuorumCoefficient;

        _writeQuorumParamsCheckpoint(params);

        emit QuorumCoefficientSet(oldQuorumCoefficient, newQuorumCoefficient);
    }

As an example, MAX_QUORUM_COEFFICIENT can be set to 3e6, which is 3x weight for against votes, that can cover the majority of cases.

[davidbrai]

thank you for the suggestion. at most this can be labeled as low risk

[dmitriia]

My understanding that this is in between medium and high risk as proposal creation and proper livecycle is a core feature of the protocol. If it is taken away this will yield massive fund loss for all the token holders. The attack surface described mostly disables the ability to add a functional proposal.

The only reason it is Medium is that admin actions are required for that. It is not improbable as disabling the outside control of the protocol can have tangible benefits for the protocol administration.

[gzeoneth]

This would require governance to pass a proposal to set such value, which I think make this issue low risk.

[dmitriia]

It can be set by admin directly, without the voting.