Users can avoid paying fees for failed L2 transactions

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L2191-L2222 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1994-L2042

Vulnerability details

Impact

On Ethereum, even for failed transactions (execution reverted at some point) a gas fee is charged. zkSync Era adopted this principle and implemented this by not reverting the whole bootloader when a L2 transaction execution has failed. The failure/success is simply recorded at the result pointer for further processing and the transaction is finalized. This way, transaction fees are paid in either case.

However, under the hood zkSync Era uses native account abstraction, see documentation and DefaultAccount.sol, to facilitate L2 transactions. Moreover, before executing a transaction through an account, the bootloader performs a (signature) validation step which indeed reverts the whole bootloader on failure in contrast to the execution step.

As a consquence, everyone can deploy a custom account which already performs the execution within the validation step (bootloader calls validateTransaction(...) method) and does nothing during the execution step (bootloader calls executeTransaction(...) method).
This results in the following behaviour:

• On execution success, the custom account works like the DefaultAccount and transaction fees are paid.
• On execution failure (reverted at some point), the validation step fails and therefore causes the whole bootloader to revert without any fees being paid. This leads to loss of income for Era node operators.

Alternatively, the execution could also be performed during the payment step (bootloader calls payForTransaction(...) method) to achieve the same outcome.

Proof of Concept

The following PoC demonstrates that the payment of transaction fees on execution failure can be easily avoided using the FeeSavingAccount.sol (secret gist), which performs the execution already on validation. Furthermore, the StingyContract.sol (secret gist), which serves as a (non-)reverting transaction target, is required for this PoC. Please put those contracts into ./code/system-contracts/contracts/test-contracts.

Apply the diff below to the existing DefaultAccount test suite and run bash quick-setup.sh from ./code/system-contracts/scripts to execute the PoC test cases:

diff --git a/code/system-contracts/test/DefaultAccount.spec.ts b/code/system-contracts/test/DefaultAccount.spec.ts
index 6231c34..472db18 100644
--- a/code/system-contracts/test/DefaultAccount.spec.ts
+++ b/code/system-contracts/test/DefaultAccount.spec.ts
@@ -7,14 +7,16 @@ import {
     Callable,
     L2EthToken,
     L2EthToken__factory,
-    MockERC20Approve
+    MockERC20Approve,
+    StingyContract,
+    FeeSavingAccount
 } from '../typechain-types';
 import {
     BOOTLOADER_FORMAL_ADDRESS,
     NONCE_HOLDER_SYSTEM_CONTRACT_ADDRESS,
     ETH_TOKEN_SYSTEM_CONTRACT_ADDRESS
 } from './shared/constants';
-import { Wallet } from 'zksync-web3';
+import { Wallet, utils } from 'zksync-web3';
 import { getWallets, deployContract, setCode, loadArtifact } from './shared/utils';
 import { network, ethers } from 'hardhat';
 import { hashBytecode, serialize } from 'zksync-web3/build/src/utils';
@@ -31,6 +33,8 @@ describe('DefaultAccount tests', function () {
     let callable: Callable;
     let mockERC20Approve: MockERC20Approve;
     let paymasterFlowInterface: ethers.utils.Interface;
+    let stingyContract: StingyContract;
+    let feeSavingAccount: FeeSavingAccount;

     const RANDOM_ADDRESS = ethers.utils.getAddress('0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef');

@@ -48,18 +52,15 @@ describe('DefaultAccount tests', function () {
         let paymasterFlowInterfaceArtifact = await loadArtifact('IPaymasterFlow');
         paymasterFlowInterface = new ethers.utils.Interface(paymasterFlowInterfaceArtifact.abi);

-        await network.provider.request({
-            method: 'hardhat_impersonateAccount',
-            params: [BOOTLOADER_FORMAL_ADDRESS]
-        });
+        stingyContract = (await deployContract('StingyContract')) as StingyContract;
+        feeSavingAccount = (await deployContract('FeeSavingAccount')) as FeeSavingAccount;
+        // fund custom account to cover transaction fees in case of success
+        await wallet.sendTransaction({to: feeSavingAccount.address, value: ethers.utils.parseEther("1")});
+
         bootloader = await ethers.getSigner(BOOTLOADER_FORMAL_ADDRESS);
     });

     after(async function () {
-        await network.provider.request({
-            method: 'hardhat_stopImpersonatingAccount',
-            params: [BOOTLOADER_FORMAL_ADDRESS]
-        });
     });

     describe('validateTransaction', function () {
@@ -152,6 +153,51 @@ describe('DefaultAccount tests', function () {
     });

     describe('executeTransaction', function () {
+
+        async function buildTransactionToStingyContract(nonce, wallet, functionName) {
+            const tx = await wallet.populateTransaction({
+                type: 113, // EIP-712 transaction
+                from: wallet.address,
+                to: stingyContract.address,
+                data: stingyContract.interface.encodeFunctionData('doNotRevert') // use success case for gas estimation
+            });
+            tx.data = stingyContract.interface.encodeFunctionData(functionName);
+            tx.from = feeSavingAccount.address; // use manually deployed FeeSavingAccount to execute transaction
+            tx.nonce = nonce;
+            tx.customData = { customSignature: "0x1234" }; // custom signature, see FeeSavingAccount._isValidSignature(...)
+            return utils.serialize(tx);
+        }
+
+        it.only('transaction succeeded,-> pay fees', async () => {
+            const tx = await buildTransactionToStingyContract(0, wallet, 'doNotRevert'); // successful execution
+
+            // perform transaction
+            const balanceBefore = await l2EthToken.balanceOf(feeSavingAccount.address);
+            await wallet.provider.sendTransaction(tx);
+            const balanceAfter = await l2EthToken.balanceOf(feeSavingAccount.address);
+
+            expect(balanceBefore.sub(balanceAfter)).to.be.greaterThan(0); // expect fees to be paid
+                
+        });
+
+        it.only('transaction failed -> do not pay fees', async () => {
+            const tx = await buildTransactionToStingyContract(1, wallet, 'doRevert'); // failed execution
+
+            // perform transaction
+            const balanceBefore = await l2EthToken.balanceOf(feeSavingAccount.address);
+            try {
+                await wallet.provider.sendTransaction(tx);
+                throw new Error("should never be reached");
+            }
+            catch (error) {
+                expect(error.body).to.contain('Transaction HALT: Account validation error: I do not wanna pay fees on failure, so FeeSavingsAccount helps me to make the whole bootloader revert');
+            }
+            const balanceAfter = await l2EthToken.balanceOf(feeSavingAccount.address);
+
+            expect(balanceBefore.sub(balanceAfter)).to.be.equal(0); // expect no fees to be paid
+                
+        });   
+
         it('non-deployer ignored', async () => {
             let nonce = await nonceHolder.getMinNonce(account.address);
             const legacyTx = await account.populateTransaction({

Tools Used

Manual review

Recommended Mitigation Steps

• Implement the account validation step via a staticcall from the bootloader to enforce that the execution step cannot be performed there, i.e. no state changes can be made. The DefaultAccount needs to be adapted accordingly.
• On failure during the payment step, try to transfer a fixed failure fee from the account/paymaster to the bootloader, see L2EthToken.transferFromTo(...).

Assessed type

Other

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[miladpiri]

Invalid, the operator would roll back this transaction and re-try.

[c4-sponsor]

miladpiri (sponsor) disputed

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

I agree with the Sponsor because in eth_calling the tx the operator would see it revert, this could create scenarios that make it hard to debug so I think QA is appropriate

[MarioPoneder]

Hi @GalloDaSballo,
It seems that the mechanics and impact of the present issue were misunderstood. I'll gladly take this as feedback to be more concise in future reports.
In the following, the issue is explained from another point of view by answering the sponsor's and your comment.
Concerning proof of the claims below, please refer to the PoC in the original report.

Sponsor comment:

The operator would roll back this transaction and re-try.

The given dispute seems to confirm the vulnerability, let's continue with an example.
Assume we are executing some "DeFi transaction" which can revert due to slippage or a deadline:

1. Using the native DefaultAccount.
   • Execution succeeds --> transaction fees are paid
   • Execution reverts due to slippage --> transction fees are paid
2. Using the FeeSavingAccount, see also PoC.
   • Execution succeeds --> transaction fees are paid
   • Execution reverts due to slippage --> bootloader reverts --> no transaction fees are paid
     --> operator re-try: Execution reverts due to slippage/deadline --> bootloader reverts --> no transaction fees are paid
     --> repeat

Impact:
Users of the custom FeeSavingAccount will never under any circumstances pay fees for failed transactions due to causing a revert in the account's validation or payment step on failed execution.
Since paying for failed transactions is intended and part of the operators' income on zkSync Era, the present vulnerability causes loss of the latter while creating a "successful-transactions-only chain".

No matter if the transaction is in- or excluded on the next try, the user always wins by not paying fees for failure.
Consider the loss of fee income if this would be possible on Ethereum, and users could stop failed transactions from being included in block and escape the fee payment by reverting some internal mechanism.
See also: Over 1.2 Million Ethereum Transactions Failed in May (external link to news site)

---

Judge comment:

I agree with the Sponsor because in eth_calling the tx the operator would see it revert, this could create scenarios that make it hard to debug so I think QA is appropriate

Seeing the transaction revert beforehand and not including it also does not lead to fee payment, which is normal/intended behaviour.
However, there are transactions which successfully pass eth_call / gas estimation, but then fail during real execution due to e.g. a slippage check. Such transactions are still included in a block/batch and the transaction fee is paid. The present vulnerability avoids the fee payment in these cases.

For further clarification, let's take a look on the recently processed transactions on zkSync Era, or especially this one for example. One can see that failed transactions, which are still included in a block/batch, are not rare at all. In all those cases, the transaction fee is deduced as intended (EVM compatibility). The use case of the present vulnerability is to prevent the fee payment in such instances by reverting the bootloader.
Anyone can simply take advantage of this by using the FeeSavingAccount.

Thanks for reading and have a nice day, everyone!

[miladpiri]

Generally, the scenario that the warden described is avoided by utilizing restrictions for account validation: https://era.zksync.io/docs/reference/concepts/account-abstraction.html#extending-eip4337 For instance, the example with slippage wouldn't work, because we don't allow accessing other contracts' data during validation. So yes, from the contracts' perspective it is possible, but it is a vulnerability (the operator will rollback the transaction and not even re-try, it'll just throw away the transaction from the mempool). The DDoS vector is prevented by the offchain validation methods

[MarioPoneder]

I have to respectfully disagree, according to https://era.zksync.io/docs/reference/concepts/account-abstraction.html#extending-eip4337 during the validation step:

It is allowed to call/delegateCall/staticcall contracts that were already deployed.

This allows to circumvent the data access limitation during validation by calling into another contract where the data can be accessed normally, see also PoC of #469 where the state of another contract is modified..

Moreover, see initial submission:

Alternatively, the execution could also be performed during the payment step (bootloader calls payForTransaction(...) method) to achieve the same outcome.

This explains why the described scenario is possible while it's also proven by the PoC in #469 that state-changing interactions with other contracts during the validation or payment step are not restricted.

Furthermore, throwing away such a failed transaction from the mempool is desired by the (ab-)user of this vulnerability in order to escape the fees on failure.

---

Edit: Although not pointed out in the initial submission, a transaction could also be executed through a custom paymaster since it also gets the necessary transaction data, while no meaningful restrictions can be imposed at this step, because:

While ETH is the formal fee token in zkSync, paymasters can provide the ability to exchange ERC20 tokens to ETH on the fly.

(see https://era.zksync.io/docs/reference/concepts/account-abstraction.html#paymasters)

All in alll, it seems evident that the discussed exploit cannot be easily avoided.

[GalloDaSballo]

While the other finding has stronger legs, this one ultimately will not have the same impact as any data passed by the CustomAccount would be already known, hence the validation could have been done offChain and not particular advantage is gained in performing it on zkSync

I think QA is most appropriate here, but I'll flag to do one final review