Proposal can be created if proposer owns no tokens when token supply is low

[code423n4]

Lines of code

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

Vulnerability details

Impact

At the early stage of the deployed DAO, it is possible that the following proposalThreshold 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#L466-L470

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

When calling the following propose function, if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD() is executed. If proposalThreshold() returns 0, calling propose will not revert. As a result, even if the proposer owns no token, she or he can still be able to create a proposal when the token supply is low.

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;
    }

Proof of Concept

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

    function test_CreateProposalWithZeroTokens() public {
        createUsers(1, 0);
        mintVoter1();

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

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

        // otherUsers[0] does not own any tokens
        assertEq(token.balanceOf(otherUsers[0]), 0);

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

        // the calculated proposal threshold is 0 because the token supply is low
        assertEq((token.totalSupply() * governor.proposalThresholdBps()) / 10_000, 0);

        // otherUsers[0], who does not own any tokens, can create a proposal
        vm.prank(otherUsers[0]);
        bytes32 returnedProposalId = governor.propose(targets, values, calldatas, "");

        // the proposal is created successfully
        assertEq(proposalId, returnedProposalId);
        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Pending));

        Proposal memory proposal = governor.getProposal(proposalId);

        // proposalThreshold corresponding to the proposal is 0
        assertEq(proposal.proposalThreshold, 0);

        // proposer of the proposal is otherUsers[0]
        assertEq(proposal.proposer, otherUsers[0]);
    }

Tools Used

VSCode

Recommended Mitigation Steps

A minimum proposal threshold governance configuration that is at least 1 can be added. When proposalThreshold() returns 0 because the token supply is low, calling propose could still revert if getVotes(msg.sender, block.timestamp - 1) is less than the minimum proposal threshold.

[GalloDaSballo]

Dup of #436