quorum() needs sanity check

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L475

Vulnerability details

Impact

When NFT totalSupply is low, the quorum() might be more sensitive to rounding errors. Sometimes the error could be large, or even return 0 result. The vote results based on these quorum() will deviate from the intention of governance.

Proof of Concept

Any new bidder can call createBid() to place a bid:

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

In test file, quorumThresholdBps is set to 2_000, then:

• when totalSupply of NFT is less than 5, quorum() will return 0.
• when totalSupply of NFT is 9, quorum() will return 1.

Seems the error is too big for small totalSupply and quorumThresholdBps.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• For low totalSupply, do additional checks for quorum() results, maybe round up.
• Set minimum and maximum values for quorumThresholdBps in initialize() and updateProposalThresholdBps().

[GalloDaSballo]

Dup of #604