099.md

Shambolic Banana Barbel

Medium

Successful governance proposals can fail to be executed

Summary

Proposals in Gov.sol will expire immediately after votingDuration with no buffer to execute the proposal. In most cases, this is fine, as execution happens directly from inside the vote() function. However, there are situations when the proposal can be moved into a "passed" state without a call to vote(), and in these cases, there is no buffer between the end of voting and the expiration time to execute the proposal.

Root Cause

In Gov.sol, proposals are usually executed automatically upon each vote if we've crossed the required threshold:

function vote(uint256 proposalID) external onlySequencer {
    ...

    if (_checkPassed(proposalID)) {
        _executeProposal(proposalID);
    }
}

However, there is a situation where this execution during a vote will not be triggered. Specifically, if the sequencer set shrinks after the final vote, we can move into a situation where the proposal passed, but there is no vote() transaction to trigger it. This is because _checkPassed() operates off the current sequencer set, not a cached set from the beginning of the vote:

function _checkPassed(uint256 proposalID) internal view returns (bool) {
    // checking invalidate votes
    address[] memory latestSequencerSet = ISequencer(SEQUENCER_CONTRACT).getSequencerSet2();
    uint256 validVotes = 0;
    for (uint256 i = 0; i < latestSequencerSet.length; i++) {
        if (votes[proposalID].contains(latestSequencerSet[i])) {
            validVotes = validVotes + 1;
        }
    }
    return validVotes > (latestSequencerSet.length * 2) / 3;
}

In this case, the call to executeProposal() will be required for the proposal to be executed.

function executeProposal(uint256 proposalID) external {
    (bool finished, bool passed, ) = proposalStatus(proposalID);
    require(!finished, "voting has ended");
    require(passed, "proposal has not been passed yet");

    _executeProposal(proposalID);
}

However, this requires that the vote is not finished, where finished is defined as executed || expirationTime == 0 || expirationTime < block.timestamp, and expirationTime is votingDuration after the vote was created (set as 1 day in the config).

In other words, if a vote becomes "passed" by a change in the makeup of the sequencer set and then we cross 1 day since the start time without calling executeProposal() (this could happen arbitrarily soon after the vote passes), it is no longer able to be executed. This can reasonable cause approved votes to become non-executable on an unreasonable timeframe.

Internal Preconditions

1. A vote must pass by a change in the sequencer set, as opposed to a new vote.

External Preconditions

1. No user must call executeProposal() in the (arbitrarily short) time before the vote expires.

Attack Path

1. A vote is created at timestamp 0.
2. Out of a group of 6 sequencers, 4 vote for it (not enough for > 2/3 quorum).
3. The sequencer set removes one of the members who voted no at timestamp 86,340 (1 minute before expiry).
4. The vote is passed, but nobody calls executeProposal() in the next minute.
5. The vote expires at timestamp 86,400, and the proposal is no longer executable.

Impact

Successful votes can become non-executable in an arbitrary short timeframe.

PoC

N/A

Mitigation

Create an additional executionTime with an extra buffer after the vote ends to allow time for the proposal to be executed.