c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L194-L197 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/bridge/L1WethBridge.sol#L181-L184 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L309-L314

Vulnerability details

Impact

If either a contract or a multisig uses the legacy deposit in any of the bridges or calls Mailbox, function requestL2Transaction with _refundRecipient == address(0), then the destination refund recipient will be its aliased address. As the address derivation is different in zkSync Era, then, for the aliased address to be user-controlled, it would need to find the preimage of such address or find a collision, which is not feasible, so the contract may be losing the refundGas or the whole bridged ETH if the transaction did fail on zkSync Era.

Proof of concept

Let's start with the bridges (I will work with the ERC20 one, the WETH one is the same). If the caller tries to make a legacy deposit

L1ERC20Bridge, lines 144 to 152

    function deposit(
        address _l2Receiver,
        address _l1Token,
        uint256 _amount,
        uint256 _l2TxGasLimit,
        uint256 _l2TxGasPerPubdataByte
    ) external payable returns (bytes32 l2TxHash) {
        l2TxHash = deposit(_l2Receiver, _l1Token, _amount, _l2TxGasLimit, _l2TxGasPerPubdataByte, address(0));
    }

it will set the _refundRecipient to 0 (the last argument) and the "real" deposit function will do

L1ERC20Bridge, lines 194 to 197

    function deposit(
        address _l2Receiver,
        address _l1Token,
        uint256 _amount,
        uint256 _l2TxGasLimit,
        uint256 _l2TxGasPerPubdataByte,
        address _refundRecipient
    ) public payable nonReentrant senderCanCallFunction(allowList) returns (bytes32 l2TxHash) {

        ...

        // If the refund recipient is not specified, the refund will be sent to the sender of the transaction.
        // Otherwise, the refund will be sent to the specified address.
        // If the recipient is a contract on L1, the address alias will be applied.
        address refundRecipient = _refundRecipient;
        if (_refundRecipient == address(0)) {
            refundRecipient = msg.sender != tx.origin ? AddressAliasHelper.applyL1ToL2Alias(msg.sender) : msg.sender;
        }

        ...
    }

That means, if our caller is a contract or a multisig, refundRecipient will be set to the alias of the caller's address. The same happens if he tries bridging ETH directly through Mailbox:

Mailbox, lines 309 to 314

    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) {

        ...

        // If the `_refundRecipient` is not provided, we use the `_sender` as the recipient.
        address refundRecipient = _refundRecipient == address(0) ? _sender : _refundRecipient;
        // If the `_refundRecipient` is a smart contract, we apply the L1 to L2 alias to prevent foot guns.
        if (refundRecipient.code.length > 0) {
            refundRecipient = AddressAliasHelper.applyL1ToL2Alias(refundRecipient);
        }

        ...

    }

The issue is that refundRecipient may not be controlled by the caller because, if that were true, then the contract caller should have found the preimage of its aliased address on zkSync Era taking into account that even the address derivation is different, so the same bytecode will have two completely different addresses on Ethereum and zkSync Era

ContractDeployer, lines 92 to 123

    function getNewAddressCreate2(
        address _sender,
        bytes32 _bytecodeHash,
        bytes32 _salt,
        bytes calldata _input
    ) public view override returns (address newAddress) {
        // No collision is possible with the Ethereum's CREATE2, since
        // the prefix begins with 0x20....
        bytes32 constructorInputHash = EfficientCall.keccak(_input);

        bytes32 hash = keccak256(
            bytes.concat(CREATE2_PREFIX, bytes32(uint256(uint160(_sender))), _salt, _bytecodeHash, constructorInputHash)
        );

        newAddress = address(uint160(uint256(hash)));
    }

    function getNewAddressCreate(
        address _sender,
        uint256 _senderNonce
    ) public pure override returns (address newAddress) {
        // No collision is possible with the Ethereum's CREATE, since
        // the prefix begins with 0x63....
        bytes32 hash = keccak256(
            bytes.concat(CREATE_PREFIX, bytes32(uint256(uint160(_sender))), bytes32(_senderNonce))
        );

        newAddress = address(uint160(uint256(hash)));
    }

which means the odds of finding a collision are near (quasi-exactly) zero. This is bad as bootloader sends the excess of gas of the requested L1 ⇒ L2 transaction to such address and, if the transaction failed, the whole ETH (sub the operator pay):

bootloader, lines 937 to 959

                let toRefundRecipient
                switch success
                case 0 {
                    // If the transaction reverts, then minting the msg.value to the user has been reverted
                    // as well, so we can simply mint everything that the user has deposited to 
                    // the refund recipient

                    toRefundRecipient := safeSub(getReserved0(innerTxDataOffset), payToOperator, "vji")
                }
                default {
                    // If the transaction succeeds, then it is assumed that msg.value was transferred correctly. However, the remaining 
                    // ETH deposited will be given to the refund recipient.

                    toRefundRecipient := safeSub(getReserved0(innerTxDataOffset), safeAdd(getValue(innerTxDataOffset), payToOperator, "kpa"), "ysl")
                }

                if gt(toRefundRecipient, 0) {
                    let refundRecipient := getReserved1(innerTxDataOffset)
                    // Zero out the first 12 bytes to be sure that refundRecipient is address.
                    // In case of an issue in L1 contracts, we still will be able to process tx.
                    refundRecipient := and(refundRecipient, sub(shl(160, 1), 1))
                    mintEther(refundRecipient, toRefundRecipient, false)
                }

so contracts using the legacy deposit may be losing either the gas refund or their entire assets if the transaction failed on zkSync Era.

Recommended Mitigation steps

Upgrade both bridges and Mailbox to revert if such deposits are made.

Assessed type

Other

c4-pre-sort commented 11 months ago

bytes032 marked the issue as duplicate of #827

c4-judge commented 10 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-judge commented 9 months ago

This previously downgraded issue has been upgraded by GalloDaSballo

c4-judge commented 9 months ago

GalloDaSballo marked the issue as satisfactory

c4-judge commented 8 months ago