search

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

3 stars 0 forks source link

ETH can be forcibly sent to any L2 contract that doesn't have any code deployed on L1 - undocumented attack vector #44

Closed c4-submissions closed 8 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L953-L959 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L310-L314 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/zksync/contracts/bridge/L2WethBridge.sol#L120

Vulnerability details

Description

zkSync Era allow user to request transaction from L1 to L2 using a component called the Mailbox facet. Such transaction happen in 2 steps (2 different transactions), firstly L1 side which will create the transaction and submit it to L2, and then when L2 pick it up and execute it. Step 2 is asynchron. In case the transaction fails on L2 for whatever reasons, the ETH sent during the transaction at L1 level are not refunded on L1, but on L2, that is a design decision choosen by the zkSync team. So zkSync implemented a refund mechanism in order to refund the user on L2 if this happen.

This refund mechanism introduce one key drawback which I consider Medium severity, as it open the doors for griefing attack against zkSync Era protocol itself or any application running on it, which can translate to zero impact to major impact (eg: DoS, funds stuck, etc) depending on the application implementation.

Essentially, the current implementation allow an attacker to introduce similar negative side effects as what SELFDESTRUCT (opcode not implemented in zkSync) is doing on L1, BUT this is not documented anywhere.

To acheive this, we need 3 things: 1) The L2 contract that the attacker wants to attack MUST NOT have any code deployed on L1 (it could be an EOA thought) 2) Request a transaction successfully from a L1 perspective (so generating NewPriorityRequest event) 3) But make the call fails at L2 level (in bootloader) when executing the transaction, which will consequently mint ETH for the refundRecipient on L2. The easiest way to make L2 transaction fails but L1 works is to provide a bad values for address _contractL2 and/or with the bytes calldata _calldata parameters in the Mailbox::requestL2Transaction call, those are not verified on the L1 transactions and passing wrong data will surely make L2 transaction fail.

Impact

ETH can be forcibly sent to any L2 contract in zkSync Era that don't have any code deployed on L1 which can creates all sort of side effects. That translate essentially in bypassing the implementation of smart contracts that have a receive() external payable function. For example for MsgValueSimulator system contract have no code deployed on L1 (like all system contracts most likely), so could be attacked. Or L2EthToken is an active EOA on L1 and doesn't seems to be allowed to hold any as it's simulate ETH on L2, but this would allow to add some which would look weird from a dev/user looking at this contract from the explorer.

Another example, any dApp that use the OpenZeppelin Proxy would assume that receiving ETH will be delegated to the implementation (which might do some accounting or business logic), but with this attack, that would be bypassed. This contract is actually used for WETH9 on L2 (check Proxy.sol), and can be attacked as it's an EOA on L1.

Another example, one high TVL app on zkSync Era. Maverick Protocol

In their Router they have the following code. Which mean the require in receive can be bypassed, would that cause a problem to the application? I will need to investigate more, the impact of this grief attack is really a case by case basis. This contract can be attacked as it's an EOA on L1.

    receive() external payable {
        require(IWETH9(msg.sender) == WETH9, "Not WETH9");
    }

    function unwrapWETH9(uint256 amountMinimum, address recipient) public payable override {
        uint256 balanceWETH9 = WETH9.balanceOf(address(this));
        require(balanceWETH9 >= amountMinimum, "Insufficient WETH9");
        if (balanceWETH9 > 0) {
            WETH9.withdraw(balanceWETH9);
            TransferHelper.safeTransferETH(recipient, balanceWETH9);
        }
    }

   function sweepToken(IERC20 token, uint256 amountMinimum, address recipient) public payable {
        uint256 balanceToken = token.balanceOf(address(this));
        require(balanceToken >= amountMinimum, "Insufficient token");
        if (balanceToken > 0) {
            TransferHelper.safeTransfer(address(token), recipient, balanceToken);
        }
    }

    function safeTransferETH(address to, uint256 value) internal {
        (bool success, ) = to.call{value: value}(new bytes(0));
        require(success, "STE");
    }

Proof of Concept

Here is how this can happen showcasing the new wETH bridge.

1) Attacker call Mailbox::requestL2Transaction simulating a bad L1WethBridge::deposit (so using same parameters as the bridge use, but changing _contractL2 to a dummy contract) , which will make the L1 transaction successfull, but L2 fails. Also providing _refundRecipient with the L2 contract target to grief attack. Let's consider simply L2WethBridge for now as a _refundRecipient. Mailbox::_requestL2Transaction will not touch refundRecipient as not address(0) and since the attack target (L2WethBridge) will not have any contract on L1 (that is the assumption for the sake of this PoC, if this target would have code deployed on L1, it would not work). 2) At some point the transaction ^^ will be picked up and executed by the bootloader. Let's assume the call fails as planned by the attacker since he injected bad _contractL2. This will cause all the ETH that left to be minted directly to the refundRecipient, which in this case is L2WethBridge. L2WethBridge is only allowed to receive ETH when called by l2WethAddress and should revert otherwise, but here those ETH will be added to L2WethBridge without any problem (even if not coming from l2WethAddress) and without emiting EthReceived event.

As you can see the revert is bypassed, as well as the business logic, which doesn't do much grief in this case, but could have a bigger impact depending on the application or system contract involved.

Another alternative to do this even in a more simpler way is to send a valid transaction for both L1 and L2 perspective, specifying as always the L2 refundRecipient to attack, but sending more ETH, the excess will be refunded to refundRecipient and cause the same behavior.

Tools Used

Manual inspection and Remix todo some testing.

Recommended Mitigation Steps

While receiving ETH forcibly is possible on L1 due to mainly SELFDESTRUCT, the fact that zkZync doesn't implement SELFDESTRUCT is on purpose, and probably since it's deprecated from L1 (and recently EIP-6780 being implemented in go-ethereum), this could translate in dApp developers thinking this cannot happen on zkSync Era, which could create a false sense of security and bad surprises for dApp developers as their implementation might not account for this invariant to be broken. Also doing this attack on L1 involve deploying a contract and is more involving then in zkSyncEra to simply submit special crafted L1 -> L2 transaction or simply a valid L1 -> L2 transaction with excess ETH.

At first glance, it doesn't seem like this grief attack could do any harm to the zkSync system contracts, but I'll let zkSync team confirm this. It's hard to understand the full impact of this attack as this is a per application basis. Nevertheless, for any of the current or future application, that is always a potential issue that developers and auditors might miss and SHOULD be warned accordingly, even if, granted, this is also a responsability of the dApps developer.

The fact that this is not documented anywhere probably means that zkSync team is not aware of this grief attack either. Additionally, from the documentation from the first zkSync C4 contest the documentation state the following regarding the L1Bridge (which is now renamed to L1WethBridge) which concurs (since it's not true) also to the thesis that it's an unknown issue:

The ether bridge is special because it is the only place where native L2 ether can be minted.

Furthermore, the zkSync team seems to think only directETHTransfer can do such behavior (when refunding excess gas for example), when it's not the case as show in my submission.

There should be a big warning about this here at least.

The root problem is really caused by the L2 aliasing logic. It's clear that if the refund addess is omitted, the sender will be used as refund, and aliasing is clear as we know the sender is coming from L1, so we alias if it's a contract. The problem with the refund address is that the implemention assume it's an L1 based recipient, but the hack here is that the user could use an L2 recipient directly instead, and there is no way to detect this really.

Assessed type

Other

c4-pre-sort commented 11 months ago

bytes032 marked the issue as primary issue

c4-pre-sort commented 11 months ago

bytes032 marked the issue as sufficient quality report

miladpiri commented 10 months ago

This is not an issue. What is possible on Ethereum is possible on zkSync too.

c4-sponsor commented 10 months ago

miladpiri (sponsor) disputed

c4-judge commented 10 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 10 months ago

I thought about this a bit as I think it could be fair for contracts on L2 not to expect receiving ETH

However, currently SC design requires expecting the possibility of receiving ETH via selfdestruct, we may have a different discussion if that was removed for a while and contracts started to optimize around that

dontonka commented 10 months ago

This is not an issue. What is possible on Ethereum is possible on zkSync too.

The point here I'm making here is the fact that this is undocumented.

I thought about this a bit as I think it could be fair for contracts on L2 not to expect receiving ETH

However, currently SC design requires expecting the possibility of receiving ETH via selfdestruct, we may have a different discussion if that was removed for a while and contracts started to optimize around that

@GalloDaSballo Please answer the following question erasing before hand any memories of my current submission XD:

While reading the developer doc and ETH vs zkSyncEra differences , do you understand that you can forcibly push ETH on a purely zkSyncEra SC?

Answer is most likely No, and that will be for the majority of devs, as Selfdestruct is not supported and clearly documented, and there is no such thing as validator rewards (that can be pushed to random address) on zkSyncEra, and since the issue I'm reporting is undisclosed information, you have no reasons to think this can be done.

So if you are a dev that build a dApp that will live purely on zkSyncEra, the SC design can definatelly be made assuming ETH cannot be forcibly pushed to your SC, which could be potentially a critical mistake as explained in my submission, as you could have hard business logic in receive() external payable, not only soft one, which could be bypassed.

For disclosure, the current issue was submitted during the contest to Immunefi, and unfortunatelly the zkSync team was unable to clearly proove that this issue was known, and while I tried to indicate that some of the currently deployed application could be vulnerable, they didn't ackknowledge any responsabilities and pushed this fully to fhe dApp developer responsability, for which I stopped wasting time hunting, as clearly even if I could establish a clear vulnerability du to this in a dApp, they would not assume any responsabilities, which doesn't seems fair.

To summarize, here is why this is justified as Medium from my perspective:

If you feel the above deserve QA, I'll respect you decision after all.

GalloDaSballo commented 9 months ago

After reviewing the findings, and the public info available

Especially after reading

The fact that this is not documented anywhere probably means that zkSync team is not aware of this grief attack either. Additionally, from the documentation from the first zkSync C4 contest the documentation state the following regarding the L1Bridge (which is now renamed to L1WethBridge) which concurs (since it's not true) also to the thesis that it's an unknown issue:

I believe the finding should remain as QA as it has to be considered as known