Balance check can revert a transaction when account has enough balance

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/DefaultAccount.sol#L103 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/libraries/TransactionHelper.sol#L410 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/bootloader/bootloader.yul#L711 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/bootloader/bootloader.yul#L1033-L1055

Vulnerability details

Impact

EIP-1559 transactions can be reverted while the account has enough funds to transfer msg.value and pay the gas fee.

Proof of Concept

During transaction validation, there's a check that ensures that the account that initiated the transaction has enough funds to send msg.value and pay the gas fee. The check calls the TransactionHelper.totalRequiredBalance to compute the minimal balance the account must have to execute the transaction. The amount is calculated as:

requiredBalance = _transaction.maxFeePerGas * _transaction.gasLimit + _transaction.value;

Notice that the maximal fee per gas value (_transaction.maxFeePerGas) is used as the gas price value. However, in the bootloader, the gas price is set to the base fee:

1. when processing of a transaction begins, gasPrice is set to the value returned by getGasPrice;
2. getGasPrice always returns the base fee, while ensuring that the maximal fee and the maximal priority fee are set correctly;
3. the gasPrice is then passed to l2TxValidation, ZKSYNC_NEAR_CALL_validateTx, and ensurePayment;
4. in ensurePayment, requiredETH is calculated by multiplying the gasPrice by the gas limit;
5. requiredETH is the transaction fee amount the bootloader expects to receive from the account.

EIP-1559 lets users set a maximal fee per gas that's higher than the base fee, to give a higher priority to their transactions. As per the specification of the project, zkSync Era supports EIP-1559 transaction, thus the value of maxFeePerGas can be greater than the base fee. As a result, DefaultAccount._validateTransaction will require a higher balance than is required by the bootloader to execute a transaction.

Consider this example: user sends a transaction and sets the gas limit to 100,000, maxFeePerGas to 12 gwei, and value to 1 ETH. DefaultAccount._validateTransaction will require that the balance of the user is:

12 gwei * 100,000 + 1 ETH = 12e9 * 1e5 + 1e18 = 1001200000000000000 = 1.0012e18

However, in this example, the base fee is only 10 gwei, thus the transaction will only transfer:

10 gwei * 100,000 + 1 ETH = 10e9 * 1e5 + 1e18 = 1001000000000000000 = 1.001e18

As a result, the transaction will revert.

Tools Used

Manual review

Recommended Mitigation Steps

In the bootloader and TransactionHelper, consider using the same value for the gas price. This is applicable to both TransactionHelper.payToTheBootloader and TransactionHelper.totalRequiredBalance.

Fixing this issue will also fix another minor issue in the bootloader: the transaction fee that's paid by accounts is always higher than the fee required by the bootloader, when the maximal fee is bigger than the base fee. As a result, the bootloader is always forced to refund excessive amounts in such transactions.

[miladpiri]

It is a known issue, and will be solved in future. Moreover, I can not agree with the conclusion "As a result, the transaction will revert.".

The impact is low, so the severity should be downgraded to Low.

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[GalloDaSballo]

@miladpiri can you please clarify where the issue was disclosed?

My understanding is that in some cases , maxFeePerGas>baseFee`, validation would revert even though the account has enough funds to pay for the tx.

Am I missing something?

[GalloDaSballo]

After thinking about it:

• The check is correct, you must have maxFeePerGas just like a normal tx

• The bootloader is making the caller pay baseFee which is the cost of gas for the current block

This means the caller is not paying any portion of the Priority Fee (I guess subsidized by ZkSync)

But it doesn't seem to create any inconsistent state nor issue

Wdyt @miladpiri ?

[StanislavBreadless]

@GalloDaSballo

Yes, it is intended we don't charge the priority fee. I wouldn't call it subsidized, because the baseFee is already enough to cover our expenses. For now since we use the FIFO approach towards processing blocks (i.e. no decentralization), there is no point in the priority fee, so we simply don't charge the users for it.

[GalloDaSballo]

With the information provided in the report:

• The Tx will be validated for maxFee and will pay baseFee, this is consistent with EIP-1559
• No tip / priority fee is paid, this may be relevant in specific times of network congestion

The finding is limited to discussing the user own tx reverting, this is not correct

The check is indeed checking for the maxFee which will not be necessary for execution

The discrepancy is worth flagging and I agree with Low Severity

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

I recommend the Warden to further model the economic implications and follow up either with me or the Sponsor

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo marked the issue as grade-c

[Jeiwan]

@miladpiri @GalloDaSballo @StanislavBreadless

My bad! I didn't add the exact lines where it reverts (although I linked the code in the Lines of code section and in the PoC). Here: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/DefaultAccount.sol#L103

So, everything you said above is true. But, DefaultAccount will require that users have funds to pay gas at maxFeePerGas and will revert when this is not true. First, it calculates the balance required to transfer msg.value and pay gas: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/DefaultAccount.sol#L102 The gas cost is calculated using _transaction.maxFeePerGas, not baseFee: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/libraries/TransactionHelper.sol#L410 It'll then revert when user's balance is not enough: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/DefaultAccount.sol#L103

Yes, it is intended we don't charge the priority fee.

I think this comment by @StanislavBreadless proves the validity of the finding. Since the priority fee is never charged, it shouldn't cause reverting of user transactions (or, more generally, impact users in any other way).

[GalloDaSballo]

I am thinking that:

• When validating, the baseFee is unknown, checking for maxFeePerGas is correct at that time
• When executing / settling, the baseFee is known, enforcing the requirement of having maxFeePerGas is not necessary, as you only need baseFee to pay for the tx.

In practical terms, I believe that the scenario described by @Jeiwan should not happen for this reason as otherwise the tx would be rejected by the sequencer.

Meaning that the change / tweak of the check can be seen more as a QA / Gas change, more so than a vulnerability or bug

@StanislavBreadless what do you think?

[StanislavBreadless]

@GalloDaSballo @Jeiwan

1. If the transaction reverts in the validation (paying for the fee is just a part of validation), then the user will not pay any fee. The most that could happen for the user is inconvenience of the sequencer not accepting the transaction as @GalloDaSballo has rightfully pointed out.
2. It is done the way it is done now, because when the validation needs to be done upon receiving the transaction the baseFee of the block is indeed not known, so we need to test that the user has enough funds for the worst case.

Since users do not lose any funds from such behavior the impact is QA/Low

[GalloDaSballo]

@StanislavBreadless Thank you for the insights, I agree with you, the Sequencer needs to check up to max, on execution we will need only up to base and no execution will happen if the check fails at the sequencer level.

@Jeiwan Lmk if you have a different perspective