zkSync Era - dontonka's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/governance/Governance.sol#L46 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/governance/Governance.sol#L249-L259

Vulnerability details

Description

The Governance contract is a bit special as not role based, but instead controlled by two multi-sig wallet which both have certain control over it. There is the owner (Matter Labs) and a security council formed by some external members. The reason for this design is to increase security over important operation and allow for instant operation in case of emergencies. So both parties should not trust each other blindly, and this smart contract should fill the gap to enforce this trust. This contract is very powerfull as it has the power to upgrade any smart contract on zKSync Era, from system contract to any user contract, it's like a root access on Linux, so it's critical to manage security accordingly.

I think the current implementation is too flexible and not secure enough, which defeat the purpose of having the security council. While normally this would be a Low severity submission, I feels it deserve a Medium severity as it's more a fundamental change.

1. securityCouncil allowed to be removed (set to address(0)) or initialize to address(0) at deployement time.
2. minDelay allowed to be 0.
3. updateSecurityCouncil and updateDelay can be updated by the owner only.

Impact

• owner can bypass security council.
• updateSecurityCouncil and updateDelay operation might be missed by security council.

Proof of Concept

The owner can bypass the security council in few ways:

1. securityCouncil == address(0) AND minDelay = 0: Such state would allow the owner todo instant transaction.
2. securityCouncil == address(0) AND minDelay > 0: Such state would allow the owner todo scheduled transaction without any security counsil being able to cancel.
3. securityCouncil != address(0) AND minDelay = 0: Such state would allow the owner todo instant transaction bypassing the security counsil.
4. securityCouncil != address(0) AND minDelay > 0: This should be the state of the contract after deployment. The security council should not trust action from the owner (that's their purpose, so any scheduleShadow should be cancel by them if not consulted before hand about what is this transaction about). When a scheduleShadow occurs, the communication about this transaction is offchain and adhoc, there is always a risk of miss information being communicated between the owner and the security council, as the owner cannot be blindly trusted, it's the reason why there is a security council. Nevertheless, the current implementation could allow the following scenario:

• ) owner decide to go rogue or keys leak. There is an important fix to be addressed which cannot go public as it could be exploited, so the owner proceed with a scheduleShadow but include 2 transactions (the security council would not see it as shadowed):
  1. minDelay = 0
  2. do the actual upgrade to fix a specific bug
• ) that could be executed instantly with the agreement of the security council (or could wait the minDelay).
• ) what happen after the bug is fixed, everyone is happy, but now the owner can execute transaction instantly, which defeat the purpose of the security council (as they would not have the time to cancel). The security council could detect this soon after as ChangeMinDelay event could be detected or directly checking the value of minDelay, but this is not atomic, and there will be a window where the owner will be able todo whatever he wants instantly.

There is a reason why there is a security council, and this means the contract should enforce rules that prevent the owner to bypass them for important operation against the behavior of governance contract itself.

Tools Used

Manual inspection

Recommended Mitigation Steps

Firstly, I would recommend to prevent minDelay to be set to zero but to a minimum value instead (hardcoded to some value that make sense), as instant transaction are already possible throught the security council. It make sense for the original OZ TimelockController contract to allow a zero minimum delay, since otherwise instant operation are impossible, but with the current addition here, that feel a bit too permissive.

Furthermore, updateSecurityCouncil and updateDelay are very critical function which should always require the agreement of both the owner and the security council (which should be mandatory at construction time to be none address(0)), not a single party, and only be allowed as scheduleTransparent, so both parties understand the change involved and don't need to trust an offline communication for those changes, it's not to hide a specific exploit anyway. The fact that security council can cancel the operation is not secure enough for those operations, which is why those two operations should require the direct acceptance of both parties, and not assume if they didn't cancel it means for sure they saw the operation and agree with it.

To acheive this I would propose something similar to a 2-step ownership transfer.

1. owner call scheduleTransparent todo updateSecurityCouncil or updateDelay as usual.
2. you would have a new logic as follow for the owner and security council to accept those governance operation change and execute it.

   modifier onlySingleGovernanceTx(Operation calldata _operation) {
        bytes32 id = hashOperation(_operation);
        require(isOperationPending(id), "Operation must be pending");
	require(_operation.calls.length == 1, "Only one governance transaction per operation");
	//TODO decode _operation.calls[0].data and require selector to be IGovernance.updateSecurityCouncil or IGovernance.updateDelay
	_;
    }
	
    modifier onlyOwnerAndSecurityCouncil() {
        require(ownerAccepted != byte32(0) && ownerAccepted == scAccepted, "Owner and security council must have accepted the transaction");
        _;
    }	
	
    function ownerAcceptGovernanceTx(Operation calldata _operation) external onlyOwner onlySingleGovernanceTx {
	require(ownerAccepted == byte32(0));
	ownerAccepted = id;
        emit OwnerAcceptedGovernanceTx(id);
    }

    function securityCouncilAcceptGovernanceTx(Operation calldata _operation) external onlySecurityCouncil onlySingleGovernanceTx {
	require(scAccepted == byte32(0));
	scAccepted = id;
        emit SCAcceptedGovernanceTx(id);
    }

    function cancelGovernance(bytes32 _id) external onlyOwnerOrSecurityCouncil {
        require(isOperationPending(_id), "Operation must be pending");
        delete timestamps[_id];
	ownerAccepted = byte32(0);
	scAccepted = byte32(0);		
        emit OperationGovernanceCancelled(_id);
    }

    function executeGovernance(Operation calldata _operation) external onlyOwnerAndSecurityCouncil {
	// same as execute...
	ownerAccepted = byte32(0);
	scAccepted = byte32(0);
    }

Personally, I would probably even add such the described 2-steps acceptance for upgrade that affects user space, as this should pretty much never happen and only in case of catastrophic hacks (like this one on Ethereum), and should require direct acceptance of both parties. That would be a little bit more involved and might not be possible, but still throwing the idea.

Finally, we also need to consider the low probability but still possible that the security council is going rogue or keys leaked. Fortunatelly, the current implementation is already mitigating this by only allowing owner to schedule transactions, so even if security council can execute instant transaction, since they cannot schedule transaction themself, that is safe and secure.

Assessed type

Governance