search

code-423n4 / 2024-03-zksync-findings

2 stars 1 forks source link

L2SharedBridge.finalizeDeposit will not work for legacy bridge #50

Closed c4-bot-2 closed 8 months ago

c4-bot-2 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/zksync/contracts/bridge/L2SharedBridge.sol#L87-L91

Vulnerability details

Proof of Concept

L2SharedBridge.finalizeDeposit function is allowed to be called by l1 shared bridge or legacy bridge.

The problem is that legacy bridge is not stored during initialization and thus finalization will not work.

Impact

Finalization of legacy deposit will not work.

Tools Used

VsCode

Recommended Mitigation Steps

Save l1LegacyBridge variable.

Assessed type

Error

c4-judge commented 8 months ago

alex-ppg marked the issue as duplicate of #77

c4-judge commented 7 months ago

alex-ppg marked the issue as partial-50

rvierdiiev commented 7 months ago

hello @alex-ppg can you explain why you set partial to this report? while short, it describes which functionality will not work and why it won't and how to fix it.

alex-ppg commented 7 months ago

Hey @rvierdiiev, the submission is imprecise and lacks sufficient elaboration in comparison to its duplicate. I understand that the error is obvious, but the difference in effort between the submissions is tangible.

rvierdiiev commented 7 months ago

@alex-ppg i uderstood your point and want to add my thoughts, if possible imagine someone also created super interesting images that show the error(diagrams), then that person put even more effort and so on(this is was selected for make people to do :)). i think that it should be enough to get the problem to the judge and i believe that i did it.

don't want to argue with you by any means and will accept the decision, just think that in case of such easy bug the explanation was enough and 50% cutoff is severe.

alex-ppg commented 7 months ago

Hey @rvierdiiev, I understand diagrams and PoCs are "boilerplate" and do not necessarily provide value, but the Warden put more effort than superficial enhancements. For example, the following point in your submission is unclear:

Save l1LegacyBridge variable.

This statement does not mean anything by itself, as what is the correct configuration of the l1LegacyBridge variable? Your submission does not link the L2SharedBridge::initialize function at all so there is insufficient context to know that the _l1LegecyBridge variable is not saved during initialization. The root cause of the issue is not the one referenced by this submission, but rather the one referenced by the primary. It also uses incorrect terminology as the l1LegacyBridge variable is never saved but rather written to. The _l1LegecyBridge variable is saved to the l1LegacyBridge data location and this context is nowhere inferred by the submission.

I appreciate your PJQA contribution, but consider the 50% reduction to be fair to the primary exhibit's warden. I advise pinpointing the root cause of the issue you describe in your submission and making sure that all relevant data points are properly depicted in it without having to "infer" things from the code itself.