31355 - [SC - Low] Past Defeated Proposals Can Be Executed in the ...

Submitted on May 17th 2024 at 16:06:05 UTC by @Breeje for Boost | Alchemix

Report ID: #31355

Report type: Smart Contract

Report severity: Low

Target: https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/AlchemixGovernor.sol

Impacts:

• Manipulation of governance voting result deviating from voted outcome and resulting in a direct change from intended effect of original results

Description

Brief/Intro

The AlchemixGovernor contract uses an implementation of L2GovernorVotesQuorumFraction, which has a known vulnerability. If a proposal passes to lower the quorum requirement, all past proposals that were defeated solely due to a lack of quorum may become executable if the votes they received now meet the new, lower quorum requirement.

Vulnerability Details

The AlchemixGovernor contract inherits from L2GovernorVotesQuorumFraction, whose implementation is similar to OpenZeppelin's GovernorVotesQuorumFraction contract version 4.5.0.

Proposal Execution Process

To understand the issue, we need to examine how a proposal is executed:

1. The execute function identifies the state of the proposal by calling the state function.

    function execute(
      // SNIP
    ) public payable virtual override returns (uint256) {
      uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash, chainId);

@->     ProposalState status = state(proposalId);
        require(
            status == ProposalState.Succeeded || status == ProposalState.Queued,
            "Governor: proposal not successful"
        );
        _proposals[proposalId].executed = true;

        // SNIP
    }

1. The state function, after validation, checks two things: whether quorum was reached and whether the vote succeeded.

    function state(uint256 proposalId) public view virtual override returns (ProposalState) {
      // SNIP: Validation

@->   if (_quorumReached(proposalId) && _voteSucceeded(proposalId)) {
          return ProposalState.Succeeded;
      } else {
          return ProposalState.Defeated;
      }
    }

1. The _quorumReached function calls the quorum function with proposalSnapshot(proposalId) as the timestamp.

    function _quorumReached(uint256 proposalId) internal view virtual override returns (bool) {
        ProposalVote storage proposalvote = _proposalVotes[proposalId];

@->     return quorum(proposalSnapshot(proposalId)) <= proposalvote.forVotes + proposalvote.abstainVotes;
    }

1. The quorum function of L2GovernorVotesQuorumFraction is implemented as shown below:

    function quorum(uint256 blockTimestamp) public view virtual override returns (uint256) {
@-.     return (token.getPastTotalSupply(blockTimestamp) * quorumNumerator()) / quorumDenominator();
    }

    function quorumNumerator() public view virtual returns (uint256) {
        return _quorumNumerator;
    }

Notice the following critical point:

• While the function gets the total supply value based on the timestamp, it uses the quorumNumerator() function, which returns the current quorumNumerator value rather than the value at the time of blockTimestamp.

Consequences of the Issue

1. If quorum was not reached for a past proposal, it cannot be executed at that time and is in a defeated state.

2. If governance updates the _quorumNumerator to a lower value using updateQuorumNumerator function:

   • The past defeated proposal may now reach the quorum with the new lower requirement.

   • The proposal state can change to succeeded, making it executable again.

Impact Details

Past proposals that were defeated only due to a lack of quorum may become executable if the number of votes they received meets the new quorum requirement.

References and Mitigation

This vulnerability is recognized by OpenZeppelin with a high severity rating. They issued a security advisory for it, which can be found here.

OpenZeppelin mitigated this issue in version 4.7.2 by:

• Deprecating the direct use of _quorumNumerator in the quorumNumerator() function.

• Implementing Checkpoints for the new _quorumNumeratorHistory variable to track past values of the quorum numerator relative to time and using the value as it was at the blockTimestamp.

The changes are detailed in this commit.

Proof of Concept

Adding Test

Add the following lines in the L2Governor contract:

  function pushCalldata(bytes calldata calldataHash) public {
      _governanceCall.pushBack(keccak256(calldataHash));
  }

Add the following testPoC function in AlchemixGovernorTest.t.sol test:

    function testPoC() public {

        // 1. Initial Setup
        
        createVeAlcx(dead, 32e21, MAXTIME, false);
        createVeAlcx(admin, TOKEN_100K, MAXTIME, false);

        assertFalse(voter.isWhitelisted(usdc));

        (address[] memory t, uint256[] memory v, bytes[] memory c, string memory d) = craftTestProposal();
    
        hevm.warp(block.timestamp + 2 days); // delay

        // 2. Asserting Voting Power is less than quorum Initially

        {
            uint256 votingPower = veALCX.getVotes(dead);