We diff-audited the zksync-association/zk-governance repository at HEAD commit b2e0143 against BASE commit 8456ffb. All the resolutions mentioned in this report are contained at commit 29f0d0e.
The following files were in scope:
l1-contracts/
└─ src/
├─ EmergencyUpgradeBoard.sol
├─ Guardians.sol
├─ Multisig.sol
├─ ProtocolUpgradeHandler.sol
├─ SecurityCouncil.sol
└─ interfaces/
├─ IGuardians.sol
├─ IL2Governor.sol
├─ IPausable.sol
├─ IProtocolUpgradeHandler.sol
├─ ISecurityCouncil.sol
├─ IStateTransitionManager.sol
└─ IZkSyncEra.sol
Update:
On 2024-08-02 we diff-audited the following two changes:
The updated contracts introduced a change to handle an incorrect edge case scenario. Prior to these pull requests, the Security Council could have approved a proposal but it would be evaluated as expired after 30 days have passed if the Guardians did not approve. The changes introduced here reflect the desired expectations, i.e., if the Security Council approves, the proposal will enter either the PendingExecution or the Ready state, regardless of Guardians approval.
Furthermore, on 2024-08-08 we diff-audited the pull request #4 at commit e8206ed within the ZKsync-Association/zk-governance repository. Prior to this change, an upgrade proposal's execution unfroze all ZKsync contracts, including bridges, state transition managers and all hyperchains before executing the actual proposal. This behavior did not align with the documentation, however. This pull request was introduced to align the effects that the proposal's execution has on the protocol with the documentation, by removing the unfreeze functionality prior to executing the proposed upgrade.
This upgrade made some adjustments to upgrade the proposal life cycle as well as the L1 governing process. The following are the main changes:
veto and refrainFromVeto functions from the Guardians and, instead, introduces a LegalVetoPeriod. This legal veto period is entered into when the upgrade is started on L1 and can be extended by the Guardians from 3 days to 7 days, during which a trusted legal entity can veto the upgrade proposal off-chain. The legal veto is entirely off-chain and incentivizes the Security Council to not approve the upgrade on-chain.propose and cancel L2 proposals to any of the Governors on L2. On some L2 Governors, special roles are assigned to the L1 Guardian multisig contract that allow it to propose without having to reach token threshold and/or cancel an active proposal.The above changes culminate in the following upgrade proposal flow:
LegalVetoPeriod.LegalVetoPeriod, the Guardians can extend this time from 3 days to 7 days.LegalVetoPeriod, the state is Waiting.
ExecutionPending.Waiting and proceeds to ExecutionPending after 30 days in Waiting.Canceled after 30 days in Waiting.ExecutionPending, the proposal processes to Ready after 1 day.Ready state, the proposal can be executed and will be Done.As mostly outlined above, the following three are the primary roles in the codebase.
Guardians: An 8-member multisig that has the following capabilities:
Waiting has window passed. The approval requires a threshold of 5.Security Council: A 12-member multisig that has the following capabilities:
Emergency Upgrade Board: The Board consists of the Guardians, the Security Council, and the ZK Foundation. It has the power to perform an emergency upgrade at any time. All three parties have to sign the upgrade, where the Guardians and the Security Council have to meet a threshold of 5 and 9, respectively.
During the audit, the auditors identified the following considerations:
Canceled state after 30 days. Thus, it is assumed that the Security Council will not move it forward during the Waiting period to the ExecutionPending state.Multisig contracts do not implement a replayability measure themselves (despite having one for the (un)freezing and the L2 Governor proposing/canceling). Instead, they are dependent on the ProtocolUpgradeHandler implementation they interact with, which prevents replayability. However, if these interactions get extended in the future, the replayability countermeasure needs to be re-evaluated.Ready state, while their order of execution is not enforced. Thus, it is expected by the Guardians and the Security Council that the order of execution is as intended.The Guardians multisig on L1 has the privilege to cancel proposals of the Token and GovOps Governor contracts on L2. This is performed by encoding a call to the cancel function and sending it through the Mailbox facet. The function signature of the cancel function takes, besides the targets, values, and calldata, the proposal description as a hash.
According to the Governor's propose function, the description string is hashed as keccak256(bytes(description)). The Guardians contract, however, hashes the description as keccak256(abi.encode(description)), which is a different hash and therefore leads to a different proposal ID. Hence, instead of canceling the intended proposal, the call reverts by trying to cancel a non-existent proposal.
Note that this hash mismatch is also present in the hashL2Proposal function used to compute the proposal ID that goes into the cancel and propose message digest. Thus, the members are also signing the message for a proposal ID that does not exist in the L2 Governor.
Consider updating the description hash using bytes.
Update: Resolved at commit 09f72cb and commit 6d8d9aa.
The ProtocolUpgradeHandler enables the Security Council to freeze the protocol as an emergency response measure. This will pause the functionality of all hyperchains and the bridge. The intention with this power is to allow the Security Council to use it only once per upgrade cycle. Hence, if the council has frozen the protocol, but no upgrade has been executed, another freeze must not be allowed. However, this is not enforced. In fact, a malicious or hijacked council can keep the protocol in a frozen state by repeatedly calling softFreeze, or hardFreeze and unfreeze.
Consider adding a FreezeStatus UnfrozenUntilUpgrade which is set when unfreeze is called. As before, whenever an upgrade is executed, the freeze status is reset to None. To call softFreeze, the status has to be None, whereas for hardFreeze, the status has to be either None or Soft.
Update: Resolved at commit 40e457b and commit eb723f7.
Throughout the codebase, the following instances of incorrect documentation were identified:
softFreezeThreshold variable is said to reset "after each freeze". It should be after each soft freeze.hardFreeze can only be initiated by a fixed threshold of 9/12 members. This is not "a small threshold"._l2Proposal parameter of the proposeL2Governor function is documented to be canceled instead of being proposed._guardiansSignatures and _securityCouncilSignatures should include the signers in order to be decoded correctly instead of just the signatures.Guardians contract is enabled to propose and cancel L2 proposals as well as approve or extend the legal veto period of L1 upgrade proposals through the ProtocolUpgradeHandler. However, it cannot veto such an upgrade proposal. It is ambiguous that the Guardians can veto "the changes proposed by the Token Assembly" as it does not clarify the status of the proposal in question. Consider clarifying the veto power of the Guardians more explicitly.There are also cases of missing documentation:
IStateTransitionManger interface.TxRequest struct.Consider correcting the documentation and filling in the missing pieces to improve the clarity of the codebase.
Update: Resolved at commit 1143fd8. The ZKsync Association team stated:
Fixed. Decided not to document
IStateTransitionMangerinterface. It is also consistent with the other interface files.
The following instances could benefit from better naming:
SecurityCouncil.sol, the UNFREEZE_THRESHOLD_TYPEHASH can be renamed to UNFREEZE_TYPEHASH as it is used to form the digest for the unfreeze function and not to set the signature threshold for the unfreeze function.cancelL2Proposal and proposeL2Governor are inconsistent for interacting with the same contract. Consider naming them along their typehash as cancelL2GovernorProposal and proposeL2GovernorProposal.Canceled stage of a proposal can only be reached if it expires, being approved neither by the Security Council nor by the Guardians. Hence, consider naming it Expired.Consider implementing the above renaming suggestions to improve the clarity and readability of the codebase.
Update: Resolved at commit d7a2262.
The reinforceFreeze and reinforceUnfreeze functions loop over all hyperchain IDs to perform their respective actions. In the rare case where the execution could get stuck at a particular ID for some unforeseen reason, it would be helpful to have reinforcement functions that can target a particular hyperchain ID.
Consider adding reinforcement functions that target a particular hyperchain ID instead of looping over all the hyperchain IDs. This will help save gas and improve the execution flow of the reinforced (un)freezing process.
Update: Resolved at commit 3196750 and commit 6148bcd.
The IZkSyncEra.sol file name does not match the IZKsyncEra contract name.
To make the codebase easier to understand for developers and reviewers, consider renaming the files to match the contract names.
Update: Resolved at commit 187a3db.
The following typographical errors were identified:
Consider correcting the above typographical errors to improve the readability of the codebase.
Update: Resolved at commit db8a2d7.
The following interfaces were identified as being incomplete:
IGuardians interface is missing the cancelL2Proposal, proposeL2Governor, and hashL2Proposal functions.IProtocolUpgradeHandler interface is missing the upgradeState, updateSecurityCouncil, updateGuardians, and updateEmergencyUpgradeBoard functions.For completeness, consider adding the missing functions to the interfaces.
Update: Resolved at commit 5b67b04.
Throughout the Guardians and SecurityCouncil contracts, constants are used to declare descriptive names for the various thresholds. However, the following integers were not specifically declared as constants:
Guardians.solSecurityCouncil.solSecurityCouncil.solSecurityCouncil.solConsider defining and using constant variables instead of using literals to improve the readability of the codebase.
Update: Partially resolved at commit 85343ef. The ZKsync Association team stated:
Partially fixed. We decided not to introduce an additional constant for the EIP1271 signature threshold, as it already has a public immutable variable in the inherited
Multisig.solcontract.
Within IProtocolUpgradeHandler.sol, the following events do not have indexed parameters:
To improve the ability of off-chain services to search and filter for specific events, consider indexing event parameters.
Update: Resolved at commit 4f98280.
The cancelL2Proposal, proposeL2Governor, and hashL2Proposal functions of the Guardians contract take the L2GovernorProposal and TxRequest struct arguments from memory. However, the structs could all be handled from calldata to save on gas.
Consider using calldata instead of memory for the struct arguments in the above mentioned functions.
Update: Resolved at commit b816cfc.
The overridable STANDARD_LEGAL_VETO_PERIOD() function returns a constant value of 3 days, can be overriden to return an arbitrary uint256 value, allowing it to return a value that exceeds the EXTENDED_LEGAL_VETO_PERIOD constant value of 7 days.
Consider adding a comment as well as an enforced check to ensure that the value returned by the STANDARD_LEGAL_VETO_PERIOD function does not exceed the EXTENDED_LEGAL_VETO_PERIOD.
Update: Resolved in pull request #3 at commit aa51e3c.
This update makes several adjustments to the Guardians' responsibility in the L1 governance process along with exposing the Guardians to proposals on L2 governors. In addition, changes have been made to the freezing behavior in the protocol upgrade cycle. Particularly, the freeze cool-down period has been removed.
The audit uncovered one high-severity issue pertaining to proposal ID inconsistency with L2 governors, and one medium-severity issue pertaining to reaching an undesirable freeze state was also discovered. Various recommendations to enhance the quality and documentation of the codebase were also made.
We greatly appreciate ZKsync Association team for providing us with thorough documentation and engaging in insightful discussions with the audit team.