No commitment to data passed as input to the init contract during the execution of the diamond cut proposal

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol#L46
https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol#L61
https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol#L92
https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol#L113
https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol#L282

Vulnerability details

Description

There is a function executeDiamondCutProposal in the DiamondCutFacet contract. It checks that proposal data passed as input to this call is equal to the data that is declared when the creation of this proposal using the following logic:

require(
    s.diamondCutStorage.proposedDiamondCutHash ==
        keccak256(abi.encode(_diamondCut.facetCuts, _diamondCut.initAddress)),
    "a4"
); // proposal should be created

But there are no checks on data that is used as input to the init contract during the execution of the diamond cut proposal.

Although such functionality may be in demand for the cases when part of the input data will be known only at the stage of execution of the proposal, in most cases init input data will be known at the stage of creation of the proposal, so it will be better to include such data to the proposal hash preimage. It will prevent a user from malicious input data that can be passed when executing the proposal and even from malicious input data that can be passed by an attacker through a potential leak of the governance private key (in the case when the governance private key was stolen attacker will not have an ability to maliciously run a fast upgrade and stole assets due to notice period, but in the case when trusted governance initiated the proposal, an attacker will have the ability to execute it with malicious init contract's input data).

Recommended Mitigation Steps

Use a combination of the data that is expected to be known at the stage of creation of the proposal (including such data as a part of the proposal hash preimage) with the data that is expected to be known only at the stage of the execution of the proposal.

[miladpiri]

It is duplicate of #46 and we agree that #46 is medium severity, but this report is not well-written (low quality) and it is difficult to understand the scenario, that is why we were thinking to downgrade this to Low because of unclear statement. So, it was difficult to asses it as Medium or Low. It seems unfair to asses both #46 and #315 equally.
All in all, we are doubting assessing this issue #315 Low or Medium.
We are mostly inetending to assess this as Low, but the judge's point of view can also help us!

[c4-sponsor]

miladpiri requested judge review

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[GalloDaSballo]

It is duplicate of #46 and we agree that #46 is medium severity, but this report is not well-written (low quality) and it is difficult to understand the scenario, that is why we were thinking to downgrade this to Low because of unclear statement. So, it was difficult to asses it as Medium or Low. It seems unfair to asses both #46 and #315 equally. All in all, we are doubting assessing this issue #315 Low or Medium. We are mostly inetending to assess this as Low, but the judge's point of view can also help us!

Thank you @miladpiri , we do have a way to mark as dup but award a 50% / 25% score, will apply that modifier

[c4-judge]

GalloDaSballo marked the issue as duplicate of #46

[c4-judge]

GalloDaSballo marked the issue as partial-50

[c4-judge]

GalloDaSballo marked the issue as full credit

[GalloDaSballo]

I agree with the sponsor in making #46 primary (it get's an extra credit), but I think the submission is substantially the same:

• Malicious calldata with benevolent proposal

For this reason am marking as dup of 46

[c4-judge]

GalloDaSballo marked the issue as satisfactory