Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Malicious operator can freeze users' bridged ERC20 tokens by appending transactions after the merkle tree is full #938

Open
c4-submissions opened this issue Oct 23, 2023 · 7 comments
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) downgraded by judge Judge downgraded the risk level of this issue duplicate-853 grade-a low quality report This report is of especially low quality Q-05 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L965-L966

Vulnerability details

Impact

In Boojum we’ll have to maintain this fact. Having the priority operations’ statuses is important to enable proving failed deposits for bridges.

However, the check for numberOfL2ToL1Logs is incorrect. As a result, malicious operator can append transaction even after the merkle tree is full. User will not be able to redeem failed deposits since the proof for the failed transaction is not included in the merkle tree.

Proof of Concept

Let's focus on function publishPubdataAndClearState of L1 messenger.

    require(numberOfL2ToL1Logs <= numberOfL2ToL1Logs, "Too many L2->L1 logs");

We are requiring nothing here. So numberOfL2ToL1Logs can be larger than 2048.

    uint256 nodesOnCurrentLevel = L2_TO_L1_LOGS_MERKLE_TREE_LEAVES;
    while (nodesOnCurrentLevel > 1) {
        nodesOnCurrentLevel /= 2;
        for (uint256 i = 0; i < nodesOnCurrentLevel; ++i) {
            l2ToL1LogsTreeArray[i] = keccak256(
                abi.encode(l2ToL1LogsTreeArray[2 * i], l2ToL1LogsTreeArray[2 * i + 1])
            );
        }
    }
    bytes32 l2ToL1LogsTreeRoot = l2ToL1LogsTreeArray[0];

Later, we simply ignore logs after 2049. They will not be reflected in the root hash.

Each time when function processL1Tx is called in bootloader, a log indicating transaction result will be sent.

            // Sending the L2->L1 log so users will be able to prove transaction execution result on L1.
            sendL2LogUsingL1Messenger(true, canonicalL1TxHash, success)

A malicious operator can wait patiently until the current pending priority transactions emit more than 2048 logs in total (operator can also do it herself!) After that, if a ERC20 deposit failed, operator conclude the batch and the failed deposit can never be redeemed, since the log is not included in the merkle tree.

Tools Used

Manual

Recommended Mitigation Steps

    require(numberOfL2ToL1Logs <= 2048, "Too many L2->L1 logs");

(It's better to have a dynamic tree here)

Assessed type

Context

@c4-submissions c4-submissions added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Oct 23, 2023
jacobheun pushed a commit that referenced this issue Oct 24, 2023
@c4-pre-sort c4-pre-sort added the low quality report This report is of especially low quality label Oct 31, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as low quality report

@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #853

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Nov 25, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo changed the severity to 2 (Med Risk)

@c4-judge
Copy link
Contributor

GalloDaSballo changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Nov 25, 2023
@miladpiri
Copy link

While the attack scenario may not be fully correct (indeed more than 2048 is not possible to send on L1), the fix / the general issue is right. Low is fair.

@c4-sponsor
Copy link

miladpiri marked the issue as disagree with severity

@c4-sponsor c4-sponsor added the disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) label Nov 27, 2023
@GalloDaSballo
Copy link

Maintaining QA and I believe that a checksum on hashes would prevent this from being exploited
If the warden wishes to have this looked at again they will have to send a coded POC of bypassing all security checks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) downgraded by judge Judge downgraded the risk level of this issue duplicate-853 grade-a low quality report This report is of especially low quality Q-05 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

7 participants