QA Report

[code423n4]

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

[GalloDaSballo]

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

Logically equivalent to address(0) check, L

approveEmergencyDiamondCutAsSecurityCouncilMemberBySignature

Not sure what this means, but if it means one caller get's to approve emerengency cuts, this is a glaring security risk

Better to have config facet, in case some update is needed in the config.sol

Not convinced by this one either, ultimately it's called config but it's just a bunch of contact / base contract

L2_LOG_BYTES is not correct, it should be L2_TO_L1_LOG_SERIALIZE_SIZE

NC

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.

Not convinced in lack of detail, if you call contract X and contract X calls contract Y, then the check is necessary on both contracts

For each deposit of an ERC20 token, the information

Unclear what you'd do and were the savings would be

When a block is committed, its hash will be stored in storedBlockHashes:

See #204 L

[GalloDaSballo]

2L 1NC

[GalloDaSballo]

Report was pretty good, but the downgraded findings add a lot of points to this QA.

While the Warden has sent a few false positives, I think the value they offered for this contest warrants them winning the best QA report

[c4-judge]

GalloDaSballo marked the issue as selected for report

[c4-judge]

GalloDaSballo marked the issue as grade-a

[miladpiri]

Thanks @GalloDaSballo for your comment, that now we are looking at this report more seriously. IMO, No.2 “approveEmergencyDiamondCutAsSecurityCouncilMemberBySignature“ can be a security issue. This is what we are now implementing to make security council members be able to approve the proposal by signature. The security issue is that in case the security council members do not have access to approve on-chain, the governor must wait for their approval to bypass the notice period. In case the proposal should be executed instantly ( in case of a critical bug in our protocol), this delay can be dangerous to our protocol. By having approval by signature, the security members can approve a proposal off-chain, and the governor can execute the instant upgrade on behalf of them by using their signatures, so all-time access to the chain is not necessary for security council members. Moreover, in case a critical proposal should be executed silently, it is necessary to be able to approve by signature. Because, the governor, in one batch transaction, proposes the proposal, and approves the upgrade on behalf of the security council members by using their signatures, and then executes the proposal. Since all the above actions are done in one transaction, there is no security risk of vulnerability leak. But, in the current structure, when the governor proposes the proposal, should wait for security members’ approval, this delay can be transparent to a malicious user to investigate the governor’s proposal, and gets a clue what the vulnerability is and then exploits the protocol. This report worths to be upgraded in terms of severity! Thanks.

[miladpiri]

@GalloDaSballo I would like to know your idea about my comment above. Thanks!

[GalloDaSballo]

@GalloDaSballo I would like to know your idea about my comment above. Thanks!

Thank you for your insight, I have sent the contest to triage, took note of your feedback and am sharing with other Judges and Wardens

[GalloDaSballo]

Similarly to #48 I have shared this finding with 2 Judges and a Top warden and we all agree that this is effectively the same finding, with similar impact.

As such Low Severity is the most appropriate.

[miladpiri]

Thanks for the follow-up!