zkSync v2 contest - HE1M's results

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/4db6c596931a291b17a4e0e2929adf810a4a0eed/ethereum/contracts/zksync/facets/DiamondCut.sol#L46 https://github.com/code-423n4/2022-10-zksync/blob/4db6c596931a291b17a4e0e2929adf810a4a0eed/ethereum/contracts/zksync/libraries/Diamond.sol#L277

Vulnerability details

Impact

When the governor proposes a diamondCut, governor must wait for upgradeNoticePeriod to be passed, or security council members have to approve the proposal to bypass the notice period, so that the governor can execute the proposal.

   require(approvedBySecurityCouncil || upgradeNoticePeriodPassed, "a6"); // notice period should expire
   require(approvedBySecurityCouncil || !diamondStorage.isFrozen, "f3");

If the governor's key is leaked and noticed by zkSync, the attacker must wait for the notice period to execute the already proposed diamondCut with the malicious _calldata based on the note below from zkSync, or to propose a new malicious diamondCut. For, both cases, the attacker loses time.

NOTE: proposeDiamondCut - commits data associated with an upgrade but does not execute it. While the upgrade is associated with facetCuts and (address _initAddress, bytes _calldata) the upgrade will be committed to the facetCuts and _initAddress. This is done on purpose, to leave some freedom to the governor to change calldata for the upgrade between proposing and executing it.

Since, there is a notice period (as zkSync noticed the key leakage, security council member will not approve the proposal, so bypassing the notice period is not possible), there is enough time for zkSync to apply security measures (pausing any deposit/withdraw, reporting in media to not execute any transaction in zkSync, and so on).

But, the attacker can be smarter, just before the proposal be executed by the governor (i.e. the notice period is passed or security council members approved it), the attacker executes the proposal earlier than governor with the malicious _calldata. In other words, the attacker front runs the governor.

Therefore, if zkSync notices the governor's key leakage beforehand, there is enough time to protect the project. But, if zkSync does not notice the governor's key leakage, the attacker can change the _calldata into a malicious one in the last moment so that it is not possible to protect the project.

Proof of Concept

https://github.com/code-423n4/2022-10-zksync/blob/4db6c596931a291b17a4e0e2929adf810a4a0eed/ethereum/contracts/zksync/libraries/Diamond.sol#L277 https://github.com/code-423n4/2022-10-zksync/blob/4db6c596931a291b17a4e0e2929adf810a4a0eed/ethereum/contracts/zksync/facets/DiamondCut.sol#L46

Tools Used

Recommended Mitigation Steps

_calldata should be included in the proposed diamondCut: https://github.com/code-423n4/2022-10-zksync/blob/4db6c596931a291b17a4e0e2929adf810a4a0eed/ethereum/contracts/zksync/facets/DiamondCut.sol#L27

Or, at least one of the security council members should approve the _calldata during execution of the proposal.

No. 1

The critical parameters in initialize(...) are not set safely:

• s.governor should be set to msg.sender, because a wrong governor address will result in loss of access to all other parts, and later changing the governor to the correct address.
• _l2BootloaderBytecodeHash should be validated like L2ContractHelper.validateBytecodeHash(_l2BootloaderBytecodeHash) as in GovernanceFacet
• _l2DefaultAccountBytecodeHash should be validated like L2ContractHelper.validateBytecodeHash(_l2DefaultAccountBytecodeHash) as in GovernanceFacet

https://github.com/code-423n4/2022-10-zksync/blob/4db6c596931a291b17a4e0e2929adf810a4a0eed/ethereum/contracts/zksync/DiamondInit.sol#L39 https://github.com/code-423n4/2022-10-zksync/blob/4db6c596931a291b17a4e0e2929adf810a4a0eed/ethereum/contracts/zksync/DiamondInit.sol#L58 https://github.com/code-423n4/2022-10-zksync/blob/4db6c596931a291b17a4e0e2929adf810a4a0eed/ethereum/contracts/zksync/DiamondInit.sol#L59

No. 2

Better to have also approveEmergencyDiamondCutAsSecurityCouncilMemberBySignature, in case the security council members do not have access to ethereum blockchain or in case it is needed to approve just in one transaction by batch of signatures (to bypass the notice period).

No. 3

Better to have config facet, in case some update is needed in the config.sol. Therefore, it is not necessary to redeploy the facets that imported config (like Executor and Mailbox)

No. 4

L2_LOG_BYTES is not correct, it should be L2_TO_L1_LOG_SERIALIZE_SIZE https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/ethereum/contracts/zksync/Config.sol#L19

No. 5

It is not needed to have modifier senderCanCallFunction for the function deposit in both L1ERC20Bridge and L1ETHBridge, because they call the function requestL2Transaction in the MailBox that has already such modifier. https://github.com/code-423n4/2022-10-zksync/blob/358f38736351a8a27e325dfcb665eeba5ec02bd5/ethereum/contracts/bridge/L1EthBridge.sol#L92 https://github.com/code-423n4/2022-10-zksync/blob/358f38736351a8a27e325dfcb665eeba5ec02bd5/ethereum/contracts/zksync/facets/Mailbox.sol#L112

No. 6

For each deposit of an ERC20 token, the information of the token is read and packed and sent to L2. Even if this token is used before for deposit, again this information is sent to L2, which is waste of gas. https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/ethereum/contracts/bridge/L1ERC20Bridge.sol#L164-L169 https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/ethereum/contracts/bridge/L1ERC20Bridge.sol#L155

No. 7

When a block is committed, its hash will be stored in storedBlockHashes: https://github.com/code-423n4/2022-10-zksync/blob/358f38736351a8a27e325dfcb665eeba5ec02bd5/ethereum/contracts/zksync/facets/Executor.sol#L164 If this block is reverted, it is not removed from storedBlockHashes: https://github.com/code-423n4/2022-10-zksync/blob/358f38736351a8a27e325dfcb665eeba5ec02bd5/ethereum/contracts/zksync/facets/Executor.sol#L336 The vulnerability is that in the GettersFacet, the function storedBlockHash(...) will return the hash of a reverted block if this block number is given as its parameter, while it should return 0. https://github.com/code-423n4/2022-10-zksync/blob/358f38736351a8a27e325dfcb665eeba5ec02bd5/ethereum/contracts/zksync/facets/Getters.sol#L86