Governance.executeInstant function: operations scheduled with predecessors delay time greater than their delay time can't be executed via executeInstant

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/governance/Governance.sol#L129-L133
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/governance/Governance.sol#L204-L206
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/governance/Governance.sol#L189
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/governance/Governance.sol#L239-L241

Vulnerability details

Impact

• When the Governance contract owner schedules an operation, he must provide the delay and the operation configuration that follows the following struct:

      struct Operation {
          Call[] calls;
          bytes32 predecessor;
          bytes32 salt;
      }

  where:

  • calls is an array of Call structs, each representing a call to be made during the operation.
  • predecessor is the hash of the operation that should be executed before this operation.
  • salt is a bytes32 value used for creating unique operation hashes.

• When the security council calls executeInstant function to execute urgent operations (without checking its delay time if it has passed or not); a check is made first to ensure that the predecessor of this operation has been executed or the operation doesn't have a predecessor via _checkPredecessorDone function:

    function _checkPredecessorDone(bytes32 _predecessorId) internal view {
        require(_predecessorId == bytes32(0) || isOperationDone(_predecessorId), "Predecessor operation not completed");
    }

• So if the operation has a predecessor that has a delay greater than the operation delay ; then the _checkPredecessorDone will revert as the _predecessorId != bytes32(0) and the isOperationDone(_predecessorId) will return false; so executeInstant will not be executed on this urgent operation and this will result in disabling this function for urgent updates.

• This issue is caused because theres's no check on the predecessor state upon scheduling operations when the operation hash is created.

Proof of Concept

Scheduling an operation:

Governance.scheduleTransparent function

    function scheduleTransparent(Operation calldata _operation, uint256 _delay) external onlyOwner {
        bytes32 id = hashOperation(_operation);
        _schedule(id, _delay);
        emit TransparentOperationScheduled(id, _delay, _operation);
    }

Governance.hashOperation function

   function hashOperation(Operation calldata _operation) public pure returns (bytes32) {
        return keccak256(abi.encode(_operation));
    }

Executing an operation:

Governance.executeInstant function/L189

_checkPredecessorDone(_operation.predecessor);

Governance._checkPredecessorDone function

    function _checkPredecessorDone(bytes32 _predecessorId) internal view {
        require(_predecessorId == bytes32(0) || isOperationDone(_predecessorId), "Predecessor operation not completed");
    }

Tools Used

Manual Testing.

Recommended Mitigation Steps

Before scheduling operations: check that the operation.predecessor has a delay time (timestamps[predecessorId]) < scheduled operation delay time (timestamps[operationId]).

Assessed type

Context

[c4-pre-sort]

bytes032 marked the issue as low quality report

[miladpiri]

By design. So, shcedule an operation as predessecor again with lower delay.

[c4-sponsor]

miladpiri (sponsor) disputed

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

QA for now, will double check

[DevHals]

Hi @GalloDaSballo ,

Thank you for your time judging the contest, regarding this issue:

• consider a scenario where a scheduled proposal involves an update that due to some emergancy reasons needs to be executed instantly (e.g., modifying vital system configurations or upgrading,..); then using a predecessor with a higher delay than the proposal delay will disable this urgent action and break the intended behavior of the instant execution!

• while I acknowledge the suggestion to create another proposal with a lower delay predecessor, but this will disrupt the intended behavior of instant execution, as there will be a delay caused by re-creating the proposal.

Could you kindly consider reevaluating this aspect of the submission?
I appreciate your time and effort in revisiting the judgment.

Thanks!

[miladpiri]

@DevHals Thanks.

I do not see any issue here.

We have upgrade proposal1 (P1), and upgrade proposal2 (P2) which is dependant on P1. Since P1 has longer delay than P2, then P2 can not be executed now. So far, so good!
Now, you mention that the P2 should be executed urgently for any reason, but it can not because it is dependant on P1 (which has larger delay). So:

• We can schedule P3 similar to P1 with shorter delay, and schedule P4 similar to P2 but dependant on P3 instead of P1. Then executing P3 and then P4.
• If it is really urgent, then having such propoal dependant on the other proposals is meaningless. We immediately schedule P3 independant of other proposals, including all the necessary upgrade data, and then we execute it.

The scenario is unrealistic.

[GalloDaSballo]

I agree with the sponsor that this finding QA as it requires an admin mistake and the scenario is not realistic