Lack of access to ETH on L2 through L1->L2 transactions

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L318 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L305-L306 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L323 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1656-L1659 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L2321 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L2328-L2333 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/DefaultAccount.sol#L122-L129

Vulnerability details

Description

ETH on L2 stored on EOAs is inaccessible through L1->L2 transactions, as for such transactions msg.value is generated only "from L1 ETH", not from the active balance of the tx.origin on L2. This is so because of the logic in the Malibox::_requestL2Transaction function, which enforces msg.value of the L1->L2 transaction to be only part of ETH to be minted inside of transaction processing on L2. Bootloader maintains this system invariant as well.

DefaultAccount incorrectly assumes that L1->L2 transactions initiated by EOA cover the needed functionality of the executeTransactionFromOutside function, so it is empty.

Users will be unable to access their ETH on L2 in the case of a malicious operator that ignores L2 transactions (so if it processes only L1->L2 transactions). Also, and importantly, this violates the security invariant that enforces the ability of the users to access L2 through L1->L2 transactions, and, therefore, withdraw funds from the rollup before the upgrade in case of a scheduled malicious upgrade.

Impact

No access to ETH on L2 that is controlled by EOAs in case of malicious operator.

Violation of the "guarantee of withdraw of funds before malicious upgrade execution" security invariant.

Recommended Mitigation Steps

Add a possibility to create L1->L2 transactions that can use ETH from the L2 balance as part of the msg.value. As an alternative, you can implement the DefaultAccount::executeTransactionFromOutside function so it will contain the expected behavior, as stated in the comment above its declaration.

Assessed type

Other

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[bytes032]

malicious operator ignores L2 transaction

[miladpiri]

Warden states that if an EOA has some fund on L2, he can not withdraw it by requesting a tx from L1. The only way is to make a tx on L2. But, if the oprator is malicious and only process L1toL2 txs, not L2 txs, then user’s fund will be trapped.

The scenario requires a malicious opearot ignoring L2 txs. The main issue is that it is not possible to withdraw the fund of an account on L2 through L1 -> L2 transaction. This is duplicate of https://github.com/code-423n4/2023-10-zksync-findings/issues/827

Medium is fair.

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[c4-sponsor]

miladpiri (sponsor) confirmed

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

It seems like I may have misunderstood this findings and similar findings related to L1->L2 execution, please send me an explanation as to exactly what function is missing that prevents this from happening and I'll re-doup this category of issues

[Barichek]

@GalloDaSballo @vladbochok

The main point of the described finding is about the violation of one of the main zkSync rollup invariants. Namely, the invariant mentioned in the finding is the "ability of the users to access L2 through L1->L2 transactions, and, therefore, withdraw funds from the rollup before the upgrade in case of a scheduled malicious upgrade".

This invariant should be maintained in the system design, as L1->L2 txs are currently the only one legal way to enforce the ability of the user to access his funds in case of the malicious operator. Therefore, it is a critical point that is the only difference between the current zkSync design and the sidechain with transitions proved by zkp.

Otherwise, it wouldn't make sense to publish state diffs using on-chain calldata: such data will be required to reconstruct the rollup state and force/process transactions in case of the malicious operator, but in case the operator can ignore L2 transactions and there is no access to ETH on L2 through L1->L2 txs there is no protection against the malicious operator. This confirms that the described vulnerability is no less serious than the "ability of the operator to not publish some of the rollup state diffs" or the "ability of the operator to ignore L1->L2 transactions (in the long-term system design with escape hatch)" (both of which are high-severity findings).

While the finding #48 and its duplicates are about the incorrect logic of the refundRecipient aliasing process, this finding is about violation of one of main system invariant.

Along with the facts that most of TVL of zkSync is native ETH (link1, link2), and custom wallets are not widely adopted on zkSync, most of the value locked in the zkSync fit the described situation.

The described above confirms the fact that the described vulnerability is a high-severity issue.

[vladbochok]

Hey @GalloDaSballo @Barichek,

I agree that #48 is a different finding, that describes another issue. Also, I can confirm that this is a valid submission. That being said by @Barichek, the problem is censorship resistance protection, so user can't escape funds from the rollup using L1 -> L2 communication.

[0xunforgiven]

Hey @GalloDaSballo, @vladbochok. Thanks for reviewing my comment.

In following the above comment from @vladbochok regarding that #803 is different issue:

I just want to add that Issue #457 explain the both impacts of the issue root cause for EOA and L1-contracts:

there could be a lot of scenarios. the fact that L1-contracts can't spend and send its own balance in L2 (the balance in aliased address) is a critical bug. This bug would happen for EOA addresses to but the impact is that EOA addresses can't handle their funds from L1 but they can still create L2 transaction directly. so the impact would be that the censorship mechanism in zkSync L1 contracts (queue of L1->L2 transaction) won't work for all transactions. and users for handling their L2 balance need to create L2 transaction directly which can be censored.

In case the issues #803 (EOA impact) and #827 (L1-contracts impact) get validated as separate issues, I believe #457 should be duplicate of both of them.

I also put more comments about this issue's root cause and impacts(which are explained in #457) in comment1 and comment2

[c4-judge]

This previously downgraded issue has been upgraded by GalloDaSballo

[c4-judge]

This previously downgraded issue has been upgraded by GalloDaSballo

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

The Warden has shown how due to a lack of implementation of `executeTransactionFromOutside` a malicious operator could prevent any withdrawal by denying L2 txs

Because of the specific scoping this is valid, because of the reliance on a malicious operator, Medium Severity seems the most appropriate

[Audittens]

Hi @GalloDaSballo.

With all due respect, I believe that this issue should be classified as high-severity vulnerability.

Main possible impact of most of the already approved high-severity vulnerabilities is the possibility of the operator (and only of him) to maliciously steal funds:

• issue #1133 impact is A malicious validator could generate and submit a proof with incorrect behavior of the div instruction.
• issue #761 impact is Attacker can manipulate the sorted queue in log sorter, constraints are not strong enough and reverted l1 logs and events can still be emitted.
• issue #702 impact is Attempting to read a word that spans across the pointer bound start+offset should return zero bytes for the addresses. When offset >= length, the result should be completely zeros, so we can skip the read. However, in current implementation, this case is not handled properly so that attacker can forge arbitary read result.
• issue #697 impact is A malicious validator could generate and submit a proof with incorrect behavior of the shr instruction.
• issue #679 impact is Attacker can forge arbitary result for opcode and and or; as for xor, an overflowed UInt8 will stay in the circuit, which can lead to unexpected behavior.

While the above-mentioned reports lead to some sort of unexpected behaviours and incorrect results of execution of some of VM functionality, the current report (#803) gives the operator ability to completely freeze the ETH value stored on zkSync (with absolutely no way to prove on L1 that the operator is ignoring l2 txs).

I believe, that such possibility leads to practically the same consequences, as the ability of the operator to steal the funds -- it is actually the analogue of the game where one party (in our case it is an operator) have an unique right to completely freeze funds of the other party (in our case it is the users), along with the unique right to unfreeze the funds at any moment in the future. The optimal result of this game have (asymptotically) the same consequences as the result of the situation where the first party can easily steal the funds of the other.

Along with the mentioned facts that most of TVL of zkSync is native ETH (link1, link2) and custom wallets are not widely adopted on zkSync (so most of the value locked in the zkSync fit the described situation), I believe that this issue is a high-severity vulnerability.

[xuwinnie]

@Audittens This issue relies on the assumption that operator is centralized, but in the foreseeable future, zksync operator will go decentralized. Before the decentralization, this issue slightly increases the trust assumption. It's worth noting that currently priority queue only ensures censorship resilience( operator can still completely ignore the queue, but can't select from the queue) So even if this issue is fixed, operator can still freeze everyone's fund.This issue only lets the operator to freeze certain user's fund without disrupting the priority queue. After the decentralization, the highs you mentioned will be exploitable, while this issue will no longer exist.