Vulnerability in Refund Withdrawal Mechanism Can Lead to Fund Loss

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

This vulnerability in the L2 fund withdrawal mechanism can result in potential fund losses for users, which may vary in size depending on the specific scenario. In cases of successful L2 transactions, it leads to the loss of refund-related funds, which are typically associated with extra gas costs and, therefore, may not constitute significant financial losses. However, when L2 transactions fail, it can result in substantial financial losses as the main value intended to be transferred to the L2 destination is sent to the refund recipient. This issue poses a risk to user funds and requires immediate attention to ensure the security and integrity of the system.

Proof of Concept

When requesting an L2 transaction, if msg.sender is a contract, the sender is set to Aliased(msg.sender). Furthermore, if refundRecipient is set to address(0), it is set to sender, which is Aliased(msg.sender). https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L249 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L310

Consequently, any refunds sent to the refundRecipient on L2 will lead to a situation where the address Aliased(msg.sender) on L2 has a nonzero balance of refund. However, a challenge arises as it becomes impossible to withdraw this refund amount from the address Aliased(msg.sender) on L2. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L958

One might anticipate that to withdraw the refund amount stored at the address of Aliased(msg.sender) on L2, the same msg.sender, which is a contract, could initiate another transaction by calling the requestL2Transaction function. In this transaction, they would set the _l2Value parameter equal to the amount held at the address of Aliased(msg.sender) on L2 and msg.value to only baseCost. This approach aims to facilitate the withdrawal of _l2Value from the Aliased(msg.sender) on L2 to a specified destination, effectively achieving the desired outcome. https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L238 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L323

Nevertheless, this approach is not feasible due to the requirement that msg.value >= baseCost + _l2Value. In essence, msg.value must once again encompass _l2Value, making it impossible to set _l2Value to the amount held by Aliased(msg.sender) on L2, while msg.value is restricted to baseCost. https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L306 Moreover, when processing L1 transactions on L2, there is a requirement that msg.value should be sufficient to cover both the value and txInternalCost. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L904 Furthermore, on L2, before transferring msg.value to the destination address, the msg.value is minted for the from address, effectively not considering the prior balance of from. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1658

For a clearer understanding, let's consider an example:

Alice deploys ContractX on Ethereum and requests an L2 transaction with _l2Value = 10 ETH and msg.value = 10 ETH + 0.05 ETH, where the additional 0.05 ETH is intended to cover baseCost. Unfortunately, the transaction fails on L2, and the gas limit is fully consumed (to simplify the example). Consequently, the 10 ETH is sent to the refund recipient, which is Aliased(ContractX) on L2.

Now, to withdraw the 10 ETH, Alice assumes that she can prompt ContractX to call the requestL2Transaction function again with _l2Value = 10 ETH, msg.value = 0.05 ETH, and _contractL2 pointing to an address under Alice's control. The expectation is that the 10 ETH will be deducted from Aliased(ContractX) on L2 and transferred to _contractL2. Unfortunately, the condition require(msg.value >= baseCost + _l2Value, "mv") enforces ContractX to set msg.value >= 10 ETH + 0.05 ETH, meaning that ContractX must pay 10 ETH on L1. This amount is then minted to Aliased(ContractX) on L2, resulting in a balance of 20 ETH. Subsequently, 10 ETH is transferred from Aliased(ContractX) to _contractL2. As a result, the original 10 ETH (related to the refund of previous tx) held in this address, which Alice intended to withdraw, remains untouched, and Alice will not have access to this amount.

In summary, there is no way to escape funds from L2 via L1 -> L2 transaction. Consequently, the aliasing mechanism, alongside the refund mechanism, is not correctly implemented in the protocol. This vulnerability directly leads to the loss of users' funds.

• If the L2 transaction is successful, only the refund, which typically relates to the extra gas limit, will be sent to the refundRecipient. The loss in this scenario is limited to the refund for the remaining extra gas limit and is not substantial.

• In the event of an unsuccessful L2 transaction, the primary value that was supposed to be transferred to the destination on L2 as msg.value will be transferred to the refundRecipient. In this case, the loss encompasses not only the refund for the remaining extra gas limit but also the main value. Therefore, the potential loss can be a significant amount.

Tools Used

Recommended Mitigation Steps

A potential solution involves modifying the _requestL2Transaction function as outlined below, so that allowing to use the balance of refundRecipient on L2, instead of minting Ether again. However, it's essential to acknowledge that this approach introduces an additional vulnerability where a malicious user may request an L2 transaction with a msg.value less than _l2Value, intentionally causing a failed transaction on L2. Please note that the same mechanism should be implemented on L2 as well.

function _requestL2Transaction(
        address _sender,
        address _contractAddressL2,
        uint256 _l2Value,
        bytes calldata _calldata,
        uint256 _l2GasLimit,
        uint256 _l2GasPerPubdataByteLimit,
        bytes[] calldata _factoryDeps,
        bool _isFree,
        address _refundRecipient
    ) internal returns (bytes32 canonicalTxHash) {
        //...

        address refundRecipient = _refundRecipient == address(0) ? _sender : _refundRecipient;
        if (refundRecipient.code.length > 0) {
            refundRecipient = AddressAliasHelper.applyL1ToL2Alias(refundRecipient);
        }

        {
            params.l2GasPrice = _isFree ? 0 : _deriveL2GasPrice(tx.gasprice, _l2GasPerPubdataByteLimit);
            uint256 baseCost = params.l2GasPrice * _l2GasLimit;
            if(refundRecipient == sender){
                 require(msg.value >= baseCost, "mv2");
            } else {
                 require(msg.value >= baseCost + _l2Value, "mv");
            }

        }

    }

Assessed type

Context

[c4-pre-sort]

bytes032 marked the issue as duplicate of #1022

[miladpiri]

It leads to loss of fund, because the user can not withdraw fund in the refund recipinet address. Medium is fair. The duplicate mentioned by the pre-sorter is irrelevant.

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[c4-sponsor]

miladpiri (sponsor) confirmed

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[GalloDaSballo]

It seems to me like this scenario is constructed ad hoc, as in reality the funds on L2 are available and are recoverable, and the available amount would be lower than "expected" simply because it's inconvenient to compute the fee paid

However, if we compute the fee paid, we would know the exact amount available and we should be able to recover it

I think this finding should be considered a refactoring

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[HE1M]

@GalloDaSballo thanks for your time.

I believe this issue is a duplicate of issue-803.

This issue highlights the challenge of withdrawing refunded ETH through an L1 -> L2 transaction.

Consequently, any refunds sent to the refundRecipient on L2 will lead to a situation where the address Aliased(msg.sender) on L2 has a nonzero balance of refund. However, a challenge arises as it becomes impossible to withdraw this refund amount from the address Aliased(msg.sender) on L2.

Additionally, the impact extends beyond the loss of the refund only; it can also result in the loss of the main amount (msg.value) intended to be transferred to the destination address on L2.

If the L2 transaction is successful, only the refund, which typically relates to the extra gas limit, will be sent to the refundRecipient. The loss in this scenario is limited to the refund for the remaining extra gas limit and is not substantial.

In the event of an unsuccessful L2 transaction, the primary value that was supposed to be transferred to the destination on L2 as msg.value will be transferred to the refundRecipient. In this case, the loss encompasses not only the refund for the remaining extra gas limit but also the main value. Therefore, the potential loss can be a significant amount.

[Barichek]

@HE1M, @GalloDaSballo

With all due respect to @HE1M, I believe that his statement from the previous comment is incorrect. While issue #803 describes lack of access to all ETH stored on EOAs on L2 through L1->L2 transactions (leading to violation of one of main rollup invariants), mentioned quotes describes only the situations of inappropriately handled refundRecipient parameter and some ETH value that should or should not come to that address. So, I believe that this issue has type of question raised in issue #48, with root of this problem judged as QA.

[miladpiri]

Thanks @Barichek for the comment.

I see the root cause of both issues 374 and 803 are the same.

• This report is not just about refund recipient issue. It is clearly stating that there is no way to withdraw funds from L2 through L1 -> L2 transaction.

  In summary, there is no way to escape funds from L2 via L1 -> L2 transaction.

• This report explaines that to withdraw fund on L2 through L1 -> L2 transaction, the same amount should be provided on L1 again. So, the same amount again will be minted on L2 for from, and it will not be possible to use the prior balance of from. The reason is explained in detail:

  Nevertheless, this approach is not feasible due to the requirement that msg.value >= baseCost + _l2Value. In essence, msg.value must once again encompass _l2Value, making it impossible to set _l2Value to the amount held by Aliased(msg.sender) on L2, while msg.value is restricted to baseCost. Moreover, when processing L1 transactions on L2, there is a requirement that msg.value should be sufficient to cover both the value and txInternalCost. Furthermore, on L2, before transferring msg.value to the destination address, the msg.value is minted for the from address, effectively not considering the prior balance of from.

• Issue-803 states that if the operator is malicious that censors L2 transactions, since withdrawal is not possible through L1 -> L2 transaction, the users will not have access to their fund on L2.

• While, this issue states that if fund is transferred to the refundRecipient during processing the L1->L2 transaction on L2, the refund will be inaccessible regardless of whether the operator is malicious or not. Note that here, refundRecipient is an address that we do not have its PK (it is alias of a contract address on L1). I.e. it can not execute a withdrawal transaction directly on L2, and must execute withdrawal transaction through L1 -> L2. And since the withdrawing requires to provide the same amount again, it will not have access to the refund.

  Alice deploys ContractX on Ethereum and requests an L2 transaction with _l2Value = 10 ETH and msg.value = 10 ETH + 0.05 ETH, where the additional 0.05 ETH is intended to cover baseCost. Unfortunately, the transaction fails on L2, and the gas limit is fully consumed (to simplify the example). Consequently, the 10 ETH is sent to the refund recipient, which is Aliased(ContractX) on L2. Now, to withdraw the 10 ETH, Alice assumes that she can prompt ContractX to call the requestL2Transaction function again with _l2Value = 10 ETH, msg.value = 0.05 ETH, and _contractL2 pointing to an address under Alice’s control. The expectation is that the 10 ETH will be deducted from Aliased(ContractX) on L2 and transferred to _contractL2. Unfortunately, the condition require(msg.value >= baseCost + _l2Value, "mv") enforces ContractX to set msg.value >= 10 ETH + 0.05 ETH, meaning that ContractX must pay 10 ETH on L1. This amount is then minted to Aliased(ContractX) on L2, resulting in a balance of 20 ETH. Subsequently, 10 ETH is transferred from Aliased(ContractX) to _contractL2. As a result, the original 10 ETH (related to the refund of previous tx) held in this address, which Alice intended to withdraw, remains untouched, and Alice will not have access to this amount.

• This issue states that if an L1-> L2 transaction is successful, only the nonzero refund will be transferred to the refundRecipient that only includes the gas refund which is not a large amount. But, if the L1 -> L2 transaction is unsuccessful, gas refund as well as the main msg.value will be transferred to the refundRecipient address. So, the inaccessible refund amount can be a large value.

• Issue-48 or issue-827 states that if during using a deposit the refundRecipient is a contract, then it will be aliased to an address that is not expected. But, it does not state that withdrawing from L2 through L1->L2 is not working as expected. I have added a comment here.

As a conclusion, I see both issues 374 and 803 share the same root cause and are valid.

[Barichek]

@miladpiri

I see that this issue mentions the root cause of the vulnerability described in the issue #803.

But I do not see how this issue can be treated as the report that describes the same vulnerability as the vulnerability described in the issue #803. According to the code4rena docs (see Penalty / Award Standardization - Duplicate Report PoC Thoroughness section):

I see this issue as the one that do not contain any mention of the main impact described in the issue #803 (violation of one of main rollup invariants, censorship protection). Also, the Impact and Recommended Mitigation Steps sections contains only mentions about lack of access to refunds and value that is minted to the refund recipient account. The Proof of Concept with quotes you mentioned all describes the problem of the lack of access through L1->L2 transactions to the ETH value that is stored on the refund recipient account, not the violation of the censorship protection invariant.

Therefore, I believe that this issue cannot be designated as a duplicate of issue #803.

Moreover, as I mentioned in the previous comment, root of the problem described in the issue #374 judged as QA.

[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 duplicate of #827

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)