Double aliasing of the `refundRecipient` address by `Mailbox.requestL2Transaction` function

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L195-L197 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/bridge/L1WethBridge.sol#L182-L184 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L262-L272 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L312-L314

Vulnerability details

Impact

• L1ERC20Bridge contract is designed to allow users to deposit ERC20 tokens from Ethereum chain to zkSync Era chain by by locking funds in the contract and sending the request of processing an L2 transaction where tokens would be minted to.

• If a user wants to bridge his ERC20 tokens from L1 to L2:

1. He invokes the L1ERC20Bridge.deposit function with the following parameters:

      function deposit(
          address _l2Receiver,
          address _l1Token,
          uint256 _amount,
          uint256 _l2TxGasLimit,
          uint256 _l2TxGasPerPubdataByte,
          address _refundRecipient
      )

where the _refundRecipient is the address on L2 that will receive the refund for the transaction.

2. Then a check is made on the _refundRecipient argument if it's a zero address; then it will be set to the L2 aliased address of the msg.sender if the transaction is sent by a contract (when msg.sender != tx.origin):

   if (_refundRecipient == address(0)) {
      refundRecipient = msg.sender != tx.origin ? AddressAliasHelper.applyL1ToL2Alias(msg.sender) : msg.sender;
      }

3. Then a call to the zkSync.requestL2Transaction is sent with the updated/aliased refundRecipient address:

      l2TxHash = zkSync.requestL2Transaction{value: msg.value}(
              l2Bridge,
              0, // L2 msg.value
              l2TxCalldata,
              _l2TxGasLimit,
              _l2TxGasPerPubdataByte,
              new bytes[](0),
              refundRecipient
          );

4. This call is sent to the DiamondProxy contract where it will invoke the Mailbox.requestL2Transaction function in L1:

5. In Mailbox.requestL2Transaction function: a canonicalTxHash will be created by the _requestL2Transaction function.

6. But it was noticed in the _requestL2Transaction function that the refundRecipient will be aliased again if refundRecipient.code.length > 0:

      if (refundRecipient.code.length > 0) {
              refundRecipient = AddressAliasHelper.applyL1ToL2Alias(refundRecipient);
          }

• This will result in wrong address of the refundRecipient due to aliasing the L1 address to L2 address twice, so the added operation will not send the refund of the transaction to the correct address and the transaction initiator will lose this asset/refund.

• The same issue is spotted in L1WethBridge.deposit function.

Proof of Concept

Code Instances:

The first aliasing in L1ERC20Bridge:

L1ERC20Bridge.deposit function/L195-L197

        if (_refundRecipient == address(0)) {
            refundRecipient = msg.sender != tx.origin ? AddressAliasHelper.applyL1ToL2Alias(msg.sender) : msg.sender;
        }

The first aliasing in L1WethBridge:

L1WethBridge.deposit function/L182-L184

       if (_refundRecipient == address(0)) {
            refundRecipient = msg.sender != tx.origin ? AddressAliasHelper.applyL1ToL2Alias(msg.sender) : msg.sender;
        }

The second aliasing in Mailbox:

Mailbox.requestL2Transaction function/L262-L272

      canonicalTxHash = _requestL2Transaction(
            sender,
            _contractL2,
            _l2Value,
            _calldata,
            _l2GasLimit,
            _l2GasPerPubdataByteLimit,
            _factoryDeps,
            false,
            _refundRecipient
        );

Mailbox._requestL2Transaction function/L312-L314

    if (refundRecipient.code.length > 0) {
            refundRecipient = AddressAliasHelper.applyL1ToL2Alias(refundRecipient);
        }

Foundry PoC:

1. Add the following test file (AddressAliasHelperTest) in the code/contracts/ethereum/test/foundry/unit/concrete/Bridge/L1WethBridge/AddressAliasHelperTest.t.sol directory, where a test is set to check that aliasing the address twice will result in different L2 addresses :

   // SPDX-License-Identifier: MIT
   pragma solidity ^0.8.17;
   
   import "forge-std/Test.sol";
   import "lib/forge-std/src/console.sol";
   import {AddressAliasHelper} from "../../../../../../cache/solpp-generated-contracts/vendor/AddressAliasHelper.sol";
   
   contract AddressAliasHelperTest is Test {
      function test_Double_Aliasing_Not_Equal() public {
          address l1Address = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4; // from Remix-Ethereum IDE
          //first L2 alias:
          address l2AddressFirstAlias = AddressAliasHelper.applyL1ToL2Alias(l1Address);
   
          //second L2 alias:
          address l2AddressSecondAlias = AddressAliasHelper.applyL1ToL2Alias(l2AddressFirstAlias);
   
          console.log("l1Address", l1Address);
          console.log("l2AddressFirstAlias", l2AddressFirstAlias);
          console.log("l2AddressSecondAlias", l2AddressSecondAlias);
   
          // L1 != L2 first alias address:
          assertNotEq(l2AddressFirstAlias, l1Address);
   
          // L2 first alias address != L2 second alias address:
          assertNotEq(l2AddressFirstAlias, l2AddressSecondAlias);
   
          // L1 != L2 second alias address:
          assertNotEq(l2AddressSecondAlias, l1Address);
      }
   }

2. Test result:

   $ forge test --match-test test_Double_Aliasing_Not_Equal -vv
   [⠘] Compiling...
   [⠒] Compiling 1 files with 0.8.21
   [⠃] Solc 0.8.21 finished in 1.83s
   Compiler run successful!
   
   Running 1 test for test/foundry/unit/concrete/Bridge/L1WethBridge/AddressAliasHelperTest.t.sol:AddressAliasHelperTest
   [PASS] test_Double_Aliasing_Not_Equal() (gas: 5260)
   Logs:
   l1Address 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
   l2AddressFirstAlias 0x6C49da6a701c568545DcFCb03fcB875f56bEeEd5
   l2AddressSecondAlias 0x7d5AdA6A701c568545DcfCb03fCB875f56beffe6
   
   Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.01ms
   Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Testing & Foundry.

Recommended Mitigation Steps

Remove refundRecipient address aliasing from L1ERC20Bridge.deposit & L1WethBridge.deposit functions so that it will be only aliased to L2 address format once in the Mailbox.requestL2Transaction.

Assessed type

Context

[c4-pre-sort]

bytes032 marked the issue as duplicate of #827

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by GalloDaSballo

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid