code-423n4 / 2023-10-zksync-findings

4 stars 0 forks source link

permanent lock of ETH balance for "aliased-address of L1-contracts" in L2 #457

Closed c4-submissions closed 11 months ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L306

Vulnerability details

Impact

MailBox contract allows L1-contracts to send transactions to L2 and create transaction from their aliased address in L2. The impact of this bug is that L1-contracts can't send their ETH balance to any other addresses,spend it, use it as gas or withdraw it from L2. And ETH transferred to "L1-contracts aliased address" in L2 would be lost forever.

The root cause of the issue is that function requestL2Transaction() in MailBox when validating L2->L1 transaction checks that msg.value >= baseCost + _l2Value and makes sure that any ETH spend on L2 has sent from L1. so the contract can't spend its own balance in its aliased address in L2 and those ETHs would be lost. as it's common sense to assume that if L1->L2 transaction executed from aliased L1-contract addresses so L1-contract addresses can control his own ETH in L2 so users and projects may send ETH directly to that aliased-address or ETH would have been transferred to aliased-address with refundRecipiend functionality. so in a lot of cases there would be a lot of ETH balance in "L1-contracts aliased address" in L2 and they would be lost forever.

I believe this is High severity because:

The amount of ETH that are going to be lost is very much(direct ETH transfers to aliased addresses or through refundrecipient) and issue would happen to any L1-contract if they transfer ETH to L2 or wants to handle their user ETH in L2. the other point is that as documents points L1 contracts can create transaction in L2 and handle their addresses and user may design their projects based on this and gather ETH at aliased addresses. for example instead of sending ETH to L1 contract in L1 a user may send them to aliased address of that contract in L2.

Proof of Concept

This is part of requestL2Transaction() logic in MailBox contract:

        // Checking that the user provided enough ether to pay for the transaction.
        // Using a new scope to prevent "stack too deep" error
        {
            params.l2GasPrice = _isFree ? 0 : _deriveL2GasPrice(tx.gasprice, _l2GasPerPubdataByteLimit);
            uint256 baseCost = params.l2GasPrice * _l2GasLimit;
            require(msg.value >= baseCost + _l2Value, "mv"); // The `msg.value` doesn't cover the transaction cost
        }

the l2Vaule is the ETH value is the value of the transaction in L2 that to address is going to receive and baseCost is L2 gas cost. by this require(msg.value >= baseCost + _l2Value, "mv") makes sure that transaction cost is covered in L1 ETH. so L1-contract can't spend any of his ETH balance in its aliased address in l2.

also bootloader doesn't allow executing L1->L2 transactions when deposited ETH is lower than gas coverage + ETH value (totalDepositedETH < l2Value + gasPrice * gasLimit). This is part of processL1Tx() function in bootloader which checks the deposited ETH and gas coverage:

                let gasPrice := getMaxFeePerGas(innerTxDataOffset)
                let txInternalCost := safeMul(gasPrice, gasLimit, "poa")
                let value := getValue(innerTxDataOffset)
                if lt(getReserved0(innerTxDataOffset), safeAdd(value, txInternalCost, "ol")) {
                    assertionError("deposited eth too low")
                }

as you can see it that if totalDepositedETH < l2Value + gasPrice * gasLimit then the bootloader will revert. this check means, any ETH L1->L2 spends in L2 should have only came from L1 and again L1-contracts can't spend their ETH in their L2 aliased-address.

the fact that L1-contract can receive ETH in L2 is obvious. it can receive funds by direct transfers from others also buy refundRecipient feature.

This is the POC that show a scenario that issue would make a lot of loss funds:

  1. project A wants his users to pay 0.1 ETH to contract1(Governance contract) address in L1.
  2. because the number of users is high project ask every one to pay to contract1's aliased addresses in L2.
  3. then project A manager will create a transaction from contract1 through MailBox to withdraw contract1's aliased address balance in L2. (contract1 can call any contract with manager calldata).
  4. but MailBox won't let contract1's transaction to go to L2 and require that contract1 send those ETH from L1 and funds would be locked.

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.

Tools Used

VIM

Recommended Mitigation Steps

in MailBox only check that user pays for the gas part of L2 transaction(baseCost) and in bootloader if the form doesn't have enough balance just mark transaction as reverted.

in Bootloader only check that L1->L2 deposited ETH covers the gas of the transaction and mint (depositedETH - gasPrice * gasLimit) for the from address and try to execute the transaction. this way if ETH balance of the from with minted amount was enough to cover l2Value, transaction will still be executed. and in the end L1-contracts will be able to spend their funds in their L2 aliased-addresses.

note:

I don't have backstage access and I can't comment during Post-Judging QA duration. so if you as judge or sponsor have any question regarding the issue please contact me directly.

Assessed type

ETH-Transfer

c4-pre-sort commented 1 year ago

bytes032 marked the issue as low quality report

miladpiri commented 1 year ago

Duplicate.

c4-judge commented 11 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)

0xunforgiven commented 11 months ago

Hi @GalloDaSballo, @bytes032, @miladpiri . Thanks for reviewing my comment.

Based on the sponsor's clarification in this comment, this issue is a duplicate which is not set as duplicate. issue #457 (this) is a duplicate of #827.

However, This report shows that the impact of this bug is significantly broader than simply refunded gas. As documented in my report:

The impact of this bug is that L1-contracts can't send their ETH balance to any other addresses, spend it, use it as gas, or withdraw it from L2. And ETH transferred to "L1-contracts aliased address" in L2 would be lost forever.

I also shows that While EOAs are not affected by the loss of funds, they are still impacted by the inability to manage their ETH balances from L1, undermining censorship resistance.

Furthermore, my report provides more detailed information about the issue:

Given the comprehensive nature of my report and the broader impact of this bug, I believe issue #457 merits consideration as a primary report for this issue.

miladpiri commented 11 months ago

Agree that it is duplicate of #827

c4-judge commented 11 months ago

This previously downgraded issue has been upgraded by GalloDaSballo

c4-judge commented 11 months ago

This previously downgraded issue has been upgraded by GalloDaSballo

c4-judge commented 11 months ago

GalloDaSballo marked the issue as duplicate of #827

c4-judge commented 11 months ago

GalloDaSballo marked the issue as satisfactory

c4-judge commented 11 months ago

GalloDaSballo marked the issue as not a duplicate

c4-judge commented 11 months ago

GalloDaSballo marked the issue as unsatisfactory: Invalid

GalloDaSballo commented 11 months ago

After reviewing this Wardens submissions, and spending between 40 to 70 hours looking at their work I have determined they have abused the submissions policy and spammed the contest, for this reason am invalidating all of their submissions

GalloDaSballo commented 11 months ago

These are the stats: https://docs.google.com/spreadsheets/d/1p0F1hnNQwctHs25qG1EJ6_JO4FE9bmyDyr-O5cveKCA/edit#gid=0