Wrongly Transferring Refund to the Paymaster

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1417

Vulnerability details

Impact

This vulnerability can lead to users losing funds in the form of gas refunds, which should rightfully be returned to them. The financial impact can be particularly severe when users provide large amounts of gas for their transactions.

Proof of Concept

Consider a situation where an user initiates an L2 transaction, and a paymaster is involved to cover the transaction fee in ETH for the user. In this setup, the paymaster receives X wei of the token, equivalent to the requiredETH from the user, and subsequently forwards the requiredETH to the bootloader to cover the user's transaction fee. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1302 https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L703 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/zksync/contracts/TestnetPaymaster.sol#L50

The refund process occurs after the transaction is executed. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1101

Since the paymaster's address is non-zero (indicating that the paymaster covered the transaction fee), the refund recipient is set as the paymaster. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1423 https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1473

However, the issue here is that the refund should not be directed to the paymaster. The paymaster has already received X wei of the token in exchange for providing the requiredETH, and the refund should rightly belong to the user.

For example, if the requiredETH is 2 ETH and the user grants the paymaster an allowance of 3000 USDC (assuming 1 ETH = 1500 USDC), the paymaster transfers 3000 USDC from the user to themselves and sends 2 ETH to the bootloader for the transaction fee. Later, if it's determined that the refund amount is 0.5 ETH, the refund is mistakenly paid to the paymaster. This means the paymaster ends up with 0.5 ETH and 3000 USDC. This is incorrect because the paymaster has already received 3000 USDC for providing 2 ETH to cover the transaction fee, and the 0.5 ETH refund should rightfully be returned to the user. Essentially, the transaction fee was 1.5 ETH, and the user provided 3000 USDC to cover it, and the paymaster exchanged the 3000 USDC for 2 ETH and transferred it to the bootloader. The bootloader identifies the excess 0.5 ETH and erroneously refunds it to the paymaster.

To illustrate, consider the scenario as if the user initially exchanged 3000 USDC for 2 ETH through the paymaster and completed the exchange. Subsequently, when the user initiates a transaction using the 2 ETH to cover the fees, without involving the paymaster, any refund should rightfully be directed to the user.

The potential loss for the user can be significant in the following scenarios:

1. When the user provides a large amount of gas, exceeding the operatorTrustedGasLimit, the surplus gas is stored in the reservedGas field, indicating that it will not be used and will be refunded. In this case, the reservedGas will also be mistakenly refunded to the paymaster. This occurs because the reservedGas is added to the gasLeft to calculate the refundInGas as shown in the code below:

let refundInGas := max(operatorProvidedRefund, add(reservedGas, gasLeft))

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1456

2. If the transaction fails quickly without consuming much gas, the entire remaining gas will be refunded to the paymaster, resulting in a loss for the user.

This vulnerability highlights the importance of users being cautious when setting the gasLimit field in the Transaction structure, particularly when using a paymaster. Any nonzero refund will be wrongly directed to the paymaster, leading to potential losses for the user. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/libraries/TransactionHelper.sol#L34

Tools Used

Recommended Mitigation Steps

In the current implementation of Account Abstraction in zkSync Era, it is advisable to direct refunds to the user instead of the paymaster, although EIP-4337 may suggest to implement differently. So, in the function refundCurrentL2Transaction, the refund recipient should be the user regardless of whether a paymaster is specified or not. So, one possible solution is that the following lines be omitted, and directly setting refundRecipient := getFrom(innerTxDataOffset). https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1415-L1447

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

miladpiri (sponsor) acknowledged

[miladpiri]

The paymaster can refund the user, but this does not happen often. Potentially need to discuss and re-design specifically for our AA. Agree with warden that the paymaster is already paid for the service he is providing. So, the refund should not be paymaster’s share. Medium is fair.

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

The Warden has shown how refunds from AA txs are sent to the Paymaster, which is already paid for their services

[c4-judge]

GalloDaSballo marked the issue as selected for report

[0xunforgiven]

Hi @GalloDaSballo, @miladpiri. Thanks for reviewing my comment.

This issue claims that:

The gas refund is directed to the paymaster instead of the user, which can lead to loss of funds, especially when setting a large gas limit or when a transaction fails quickly. The fix is to direct refunds to the user instead of the paymaster.

I believe issue #620 is OOS because the following facts are documents in the code, contest's docs and zkSync website's docs: (doc1, doc2, code)

1. In the end of tx processing, bootloader refunds ETH to the paymaster.
2. bootloader notify the paymaster by paymaster.postTransaction()
3. paymaster converts ETH to ERC20 and refund the user.

As clearly stated in the documentation, Step 7 specifically addresses refunding gas fee payments using ERC-20 tokens through paymaster:

Step 7. (only in case the transaction has a paymaster) The postTransaction method of the paymaster is called. This step should typically be used for refunding the sender the unused gas in case the paymaster was used to facilitate paying fees in ERC-20 tokens.

Therefore, directing funds from bootloader to the paymaster(and to user from paymaster) is an intentional design decision, thoroughly documented across various resources. This approach relies on the user's responsibility to utilize trusted paymasters. Any potential losses incurred due to employing untrusted paymasters fall under the user's mistake. This is sponsor confirmation regarding the necessity of trusted paymasters:

Moreover, the proposal to refund remaining ETH to the user's address directly contradicts the fundamental purpose of account abstraction. One of the primary motivations for account abstraction is to cater to users who prefer holding ERC-20 tokens and paying gas fees using those tokens. Refunding ETH to such users would result in an undesirable user experience.

In conclusion, the issue raised in the report is OOS due to the well-documented design decision to direct gas refunds to the paymaster and the inherent responsibility of users to employ trusted paymasters.

[GalloDaSballo]

@miladpiri @vladbochok pls review this one

[miladpiri]

This is a design level issue. I mean, it was a design decision, but after the code4rena contest we concluded that maybe it was not the best choice, while we agree that the suggestion by the warden makes some sense. Regarding the severity, judge can decide better.

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

I think most of the content presented above was presented in a disingenous way

This line was purposefully ignored

there is no guarantee that this method will be called.

Additionally the screenshotting of the sponsor comment, out of context, with a question that is not even exactly tied to this, is disingenous

That said, the comment about using ERC20s to pay makes sense, meaning that they paymaster would have to determine that converted amount

Additionally, the paymaster code is not fully known, so it makes sense to downgrade to QA for now

[c4-judge]

GalloDaSballo marked the issue as not selected for report