Inconsistency in L1ERC20Bridge and L1WethBridge deposit could lead to user fund loss

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/72f5f16ed4ba94c7689fe38fcb0b7d27d2a3f135/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L194-L197 https://github.com/code-423n4/2023-10-zksync/blob/72f5f16ed4ba94c7689fe38fcb0b7d27d2a3f135/code/contracts/ethereum/contracts/bridge/L1WethBridge.sol#L181-L184 https://github.com/code-423n4/2023-10-zksync/blob/72f5f16ed4ba94c7689fe38fcb0b7d27d2a3f135/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L247-L250

Vulnerability details

Impact

Inconsistency check of refundRecipient in bridges contracts deposits could lead to loss of user funds during a deposit.

As it is clearly explained in the comments, refund address needs to be under least control such that if _refundRecipient:

• is a contract on L1 => refund sent to aliased _refundRecipient
• is address(0) and sender has no deployed bytecode on L1 => sent to msg.sender.
• is address(0) and sender has deployed bytecode on L1 => sent to aliased msg.sender.

Problem arise from the first statement / check which is not performed as expected.

Proof of Concept

Supposing a user deposits an amount of WETH or Tokens (depending on the bridge) and for some reason, he ends up providing a contract address as _refundRecipient argument, while the deposit on L2 fails.

Nothing in the code will actually prevent him from setting such a parameter, which will lead to loss of user funds in case of issue during the process.

Here are the checks performed so far:

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

The logic will apply the alias IF and ONLY IF _refundRecipient hasn't been set / set to address(0).

As a result, the address won't match the aliased one expected on executeL2Transaction in Mailbox contract, preventing user from accessing his refund on L2 performed by this code:

// Change the sender address if it is a smart contract to prevent address collision between L1 and L2.
// Please note, currently zkSync address derivation is different from Ethereum one, but it may be changed in the future.
address sender = msg.sender;
if (sender != tx.origin) {
    sender = AddressAliasHelper.applyL1ToL2Alias(msg.sender);
}

Here having defined a contract in deposit _refundRecipient, which won't be aliased, won't be able to call the execution on L2 as called will be applied an alias here.

Tools Used

Manual review

Recommended Mitigation Steps

In order to avoid such a case, it is recommended to standardize the logic here between the bridges and Mailbox, such that both logic matches and prevent from a gab leading to loss of funds.

If the purpose if effectively to only perform aliasing over smart contracts _refundRecipient, then quoted code blocks from both L1WethBridge and L1ERC20Bridge should be:

// Util function that will help checking if the given address is a contract / EOA or contract under construction, the context of use below will prevent such ambiguousness.
function isContract(address account) private returns (bool) {
    uint size;
    assembly {
        size := extcodesize(account)
    }
    return size > 0;
}

...

address refundRecipient;
// Same logic here
if (_refundRecipient == address(0)) {
    refundRecipient = msg.sender != tx.origin ? AddressAliasHelper.applyL1ToL2Alias(msg.sender) : msg.sender;
} else {
    // Here we can rely on extcodesize since caller would already be checked above
    refundRecipient = isContract(_refundRecipient) ? AddressAliasHelper.applyL1ToL2Alias(_refundRecipient) : _refundRecipient;
}

In the below code, we also check on the given _refundRecipient address and apply the alias if needed. Reducing the surface of errors and possibility of locking funds forever.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #827

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

After thinking about this:

• If you asked the contract to input an unaliased recipient which would then be aliased, it would be strictly worse for everyone
• The recipient is specified to the aliased contract iif the input is 0

I think QA is most appropriate because the documentation seems incorrect but I don't think a major "coding rule" was broken, the alternative seems way more difficult for integrators

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as not selected for report

[c4-judge]

This previously downgraded issue has been upgraded by GalloDaSballo

[c4-judge]

GalloDaSballo marked issue #827 as primary and marked this issue as a duplicate of 827

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)