Possible loss of funds when withdrawing from L2 to L1

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/L2EthToken.sol#L80 https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L48

Vulnerability details

Impact

Context

To initiate a withdrawal from L2 to L1, a user can call L2EthToken.withdraw method, then funds will be available to calim on L1 via finalizeEthWithdrawal method of MailboxFacet.

function withdraw(address _l1Receiver) external payable override

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/L2EthToken.sol#L80

The mechanism behind this is that thewithdraw method sends a constructed withdraw message to L1 via L1Messenger's sendToL1 method, then the user can use the message as a proof in order to claim the funds on L1.

        // Send the L2 log, a user could use it as proof of the withdrawal
        bytes memory message = _getL1WithdrawMessage(_l1Receiver, amount);
        L1_MESSENGER_CONTRACT.sendToL1(message);

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/L2EthToken.sol#L90-L91

Possible loss of funds

If the withdrawal transaction succeeds, the user should be able to claim his/her funds on L1 always. However, there is a case where the withdrawal transaction succeeds even though the message wasn't sent to L1. This could happen if there is not enough gas during message sending. As a result, the user loses funds since the ether are burnt in L2 but can not be claimed on L1 due to the failure of sending the message to L1.

Note: the withdraw method should be called via MsgValueSimulator since msg.value is set by the simulator.

Proof of Concept

The execution sequence as follows:

flowchart TD;
        subgraph L2EthToken
    withdraw-->L2EthToken._getL1WithdrawMessage(_getL1WithdrawMessage)
    end
        subgraph L1Messenger
sendToL1;       
        end
        subgraph SystemContractHelper
toL1;       
        end
        L2EthToken-->L1Messenger
        L1Messenger-->SystemContractHelper

Let's have a look at SystemContractHelper.toL1 method:

    function toL1(bool _isService, bytes32 _key, bytes32 _value) internal {
        address callAddr = TO_L1_CALL_ADDRESS;
        assembly {
            // Ensuring that the type is bool
            _isService := and(_isService, 1)
            // This `success` is always 0, but the method always succeeds
            // (except for the cases when there is not enough gas)
            let success := call(_isService, callAddr, _key, _value, 0xFFFF, 0, 0)
        }
    }

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/libraries/SystemContractHelper.sol#L48

There is call to TO_L1_CALL_ADDRESS address, please note that this is just a simulation to zkSync VM-specific v1.3.0 opcodes. For more info, check this: https://github.com/code-423n4/2023-03-zksync/blob/main/docs/VM-specific_v1.3.0_opcodes_simulation.pdf

As noticed, the method should always succeed except when there is not enough gas. In this case, the method could possibly fail silently leading to a successfull withdraw transaciton without having the message sent to L1. Since the gas limit is the minimum of operatorTrustedErgsLimit and gasLimit provided by the user, it is possible the issue occurs if both:

1. The gasLimit provided by the user was picked up because it was lower than operatorTrustedErgsLimit. https://github.com/code-423n4/2023-03-zksync/blob/main/bootloader/bootloader.yul#L1128-L1129
2. The gasLimit couldn't cover L1 messaging cost but was enough to make the withdraw transaction succeeds.

For more details about to_l1_message_opcode, have a look at eraSyncVM relevant code:

• https://github.com/matter-labs/era-sync_vm/blob/main/src/vm/vm_cycle/opcode_execution/log.rs#L16
• https://github.com/matter-labs/era-sync_vm/blob/main/src/vm/vm_cycle/opcode_execution/log.rs#L243

Tools Used

Manual analysis

Recommended Mitigation Steps

One possibility is to change the behaviour of to_l1_message_opcode to return a failure in case there wasn't enough gas and then validate it. But this fix would be on a VM level and might not be preferred. Another possibility is to add a check in L1 method to assert there is a minimum gas to cover the messaging as it's critical especially when used for bridging funds from L2 to L1.

[miladpiri]

It is not a call, but opcode compiler simulation. So, it is different from normal call.

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[GalloDaSballo]

In contrast to the "expected" behavior of call, the tx can fail due to lack of gas, but the entire tx will revert in those cases (because the call is actually transpiled to another function)

If you can reproduce the bug, please do follow up with me or the Sponsor, but with the info given I must close as invalid

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid