Centralization risks around `Governance.sol`

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/governance/Governance.sol#L167 https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/governance/Governance.sol#L154

Vulnerability details

Impact

The presence of modifiers and their implementations in the Governance.sol contract indicates that the Owner, the Security Council and the contract have distinct roles. As described in an article by Matter Labs, the purpose of the Security Council is to ensure that upgradability is not controlled by a single user or team. However, upon examining the implementation of the Governance contract, it does not fulfill this condition. There is no issue with only the Owner being able to open proposals through scheduleTransparent() or without exposing the operation on the chain through scheduleShadow(), however the fact that only the Owner can prevent the execution of proposals with cancel() appears to be a centralization issue.

With the current state of Governance.sol, the Owner has the ability to propose and execute() an operation granting unlimited authority to themselves using scheduleShadow(), because the updateDelay and updateSecurityCouncil functions, protected by onlySelf modifiers, can be successfully modified when called from within the execute() function since msg.sender = Governance.sol. In fact, even without changing the Security Council, by simply setting minDelay = 0, the Owner could make the existence of both executeInstant() and Security Council unnecessary, and instantly execute any operation they desire.

Proof of Concept

It's not possible for me to show coded PoC in the current repository, but hypothetically;

1-) Owner scheduleShadow()s an operation which gives themselves full authority by changing the securityCouncil and minDelay and even upgrading all contracts as they wil. 2-) Waiting for minDelay to pass, operation is unknown on-chain. 3-) execute()s the scheduled operation.

The snippet below shows that canceling can only be done by the Owner role.

function cancel(bytes32 _id) external onlyOwner {
        require(isOperationPending(_id), "Operation must be pending");
        delete timestamps[_id];
        emit OperationCancelled(_id);
    }

Tools Used

Manual Reveiw

Recommended Mitigation Steps

Although I do not assume that the owner could be malicious and I do not like issues related to centralization, it is stated on the contest page that "Matter Labs and is also partially trusted, it should never be able to directly steal users' funds or conduct malicious upgrades." I think the current implementation of Governance contradicts that condition, Matter Labs' philosophy, and abolishes the mission of the Security Council.

The problem can be solved in the most elegant and simple way by adding the onlyOwnerOrSecurityCouncil modifier to the cancel() function.

Assessed type

Governance

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

Article is about zkSync Lite :). Governance assumption is explicitly stated in the documentation.

[alex-ppg]

The Warden outlines centralization issues present in the codebase, however, no privilege escalation path has been demonstrated.

As the contest's documentation highlights that the governance-related members are to be considered trusted, I cannot consider this a valid vulnerability. I believe this should have been submitted as part of an Analysis report.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope

[0xpessimist]

First, in response to saxenism's statement;

As I wrote in the report, I do not assume that the owner would be malicious. The actual issue I want to point out is I thought that what was implemented in the smart contract was not actually the desired (or claimed) structure, and I will explain why in a moment. I realize the article is for zkSync Lite, just wanted to use it for explanation of the Security Council structure because zkSync still wants the Security Council to function the same way for the Governance contract in subject.

What leads me to this conclusion is that in the official and up-to-date documentation for zkSync, the Smart Contracts - Governance section claims that it should do exactly what I propose as Mitigation, but is not implemented in the code:

From: https://docs.zksync.io/zk-stack/components/smart-contracts/smart-contracts.html#governance

Please note, that both the Owner and Security council can cancel the upgrade before its execution.

Also, please note that in the "Access control and permissions" section at the very beginning of Attack Ideas on the contest page it says:

Special scrutiny should be paid to the powers of the operator. While currently the operator is controlled by Matter Labs and is also partially trusted (for instance, it is responsible for supplying the correct L1 gas price), it should never be able to directly steal users' funds or conduct malicious upgrades.

The current implementation allows malicious upgrades because the cancel() has single point of control.

And in response to alex-ppg's statement;

I clearly show the privilege escalation path both in the 2nd paragraph of Impact and in my Proof-of-Concept. Also, if you are interested in damage, I asked Vlad B about the operations that can be done with execute() and it was said that all contracts (Bridges, STs and etc) can be upgraded. Since I did not assume that owner role could be malicious, I hesitated to specify the max damage. I think my issue is related to the Governance contract within the scope and is definitely not OOS.

Lastly, after thinking about the issue for a while I thought the report title was insufficient. If possible, I would like to change the title to "Single point of control in Governance::cancel()" or "Single point of control in Governance::cancel() prevents the Security Council from fulfilling its role". Whichever the sponsor deems more appropriate.

Thank you!

[0xpessimist]

Update to summarize the comment above:

It´s stated that neither the governance nor the security councill are malicious, however, they can circumvent the limitations imposed on them. The submission shows that circumvention is possible along with the privilege escalation path shown on the Proof of Concept. It is stated by the sponsor that all contracts (Bridges, STs and etc) can be upgraded with execute() and cancellation of scheduled operations cannot be done by the Security Council because it is not implemented as stated in the document.

[alex-ppg]

Hey @0xpessimist, thanks for contributing to the PJQA process. Per the changelog shared during the contest, the change for the owner to be the sole member able to cancel a proposal is a deliberate and conscious change that the zkSync Era team was aware of. You can find it here: https://github.com/code-423n4/2024-03-zksync/blob/main/docs/Protocol%20Section/Changelog.md#l1-smart-contracts

As such, this would fall under the "known issues" clause and thus still be deemed out-of-scope regardless of whether the documentation is in sync or not. I appreciate the due diligence and still consider your remark to be a valid criticism, but I believe the functionality presently implemented is in line with what the zkSync Era desires, and this desire was explicitly stated in the contest.

If it was not, I could consider this a valid finding but I cannot in good faith assume an explicit business requirement by zkSync Era is considered an HM vulnerability when in reality it revolves around access control rather than a mathematical or economical issue. Regardless of this, I will make sure to escalate to the zkSync Era team for a re-review which will not influence the exhibit's evaluation due to the information present in the contest page.