When `totalSupply` is low `getProposalThresholdAmount()` and `getQuorumVotesAmount()` can return zero

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/cfed0dfa3bebdac0993b1b42239b4944eb0b196c/src/governance/GovernorBravoDelegateMaia.sol#L87-L93

Vulnerability details

Impact

At the early stage of the deployed DAO, it is possible that the following getProposalThresholdAmount() and getQuorumVotesAmount() returns 0 because the token supply is low.

Proof of Concept

LINK TO CODE

    function getProposalThresholdAmount() public view returns (uint256) {
        return govToken.totalSupply() * proposalThreshold / DIVISIONER;
    }

    function getQuorumVotesAmount() public view returns (uint256) {
        return govToken.totalSupply() * quorumVotes / DIVISIONER;
    }

Assume the effect when getQuorumVotesAmount() = 0:

• When determining the proposal's state, the following state function is called, which can execute else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < getQuorumVotesAmount()) { return ProposalState.Defeated; }.
• If getQuorumVotesAmount() is 0, the proposal.forVotes < getQuorumVotesAmount() 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. state()

  function state(uint256 proposalId) public view returns (ProposalState) {
      require(
          proposalCount >= proposalId && proposalId > initialProposalId, "GovernorBravo::state: invalid proposal id"
      );
      Proposal storage proposal = proposals[proposalId];
      if (proposal.canceled) {
          return ProposalState.Canceled;
      } else if (block.number <= proposal.startBlock) {
          return ProposalState.Pending;
      } else if (block.number <= proposal.endBlock) {
          return ProposalState.Active;
      } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < getQuorumVotesAmount()) {
          return ProposalState.Defeated;
      } else if (proposal.eta == 0) {
          return ProposalState.Succeeded;
      } else if (proposal.executed) {
          return ProposalState.Executed;
      } else if (block.timestamp >= add256(proposal.eta, timelock.GRACE_PERIOD())) {
          return ProposalState.Expired;
      } else {
          return ProposalState.Queued;
      }
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Make both functions return some sensible MIN_VALUE if they amount to 0.

Assessed type

Math

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c

[Nadinnnn]

• This issue is an edge case of #179 when calculating getProposalThresholdAmount() and getQuorumVotesAmount() based on govToken.totalSupply(). It could lead to arbitrary proposals being created and executed when govToken.totalSupply() is low.
• The same problem has been recognized as medium before here

[c4-judge]

This previously downgraded issue has been upgraded by trust1995

[c4-judge]

This previously downgraded issue has been upgraded by trust1995

[c4-judge]

trust1995 marked the issue as duplicate of #180

[c4-judge]

trust1995 marked the issue as satisfactory

[0xLightt]

This is not a duplicate of #180, which describes a race condition between minting bHermes and creating a proposal. This issue describes early governance stages of a protocol. In our scenario, this will never happen with bHermes and would require an error in migration to V2 to happen with Maia/vMaia.

While the mitigation is completely valid and works, having a MIN_TOTAL_SUPPLY is equivalent and easier to visualize in my opinion. The reason behind this might being a low/QA, is because what we intend to do and that completely prevents this is only calling _initiate after a certain amount of totalSupply exists. So might be argued that this can only happen due to api misuse by the owner, but adding the minimum value is still valid just to be safe.

In addition, there is the ability for the admin to veto proposals, so this further prevents anyone from using this to try and do a governance attack.

[c4-judge]

trust1995 marked the issue as not a duplicate

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)