c4-submissions commented 9 months ago

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1270-L1274

Vulnerability details

Impact

Let's begin with an overview about the cornerstones of this vulnerability. Not every point of the following list is a problem itself, but in combination they are:

zkSync Era allows custom account abstraction (an amazing feature), therefore one can build and deploy a custom account contract and the bootloader will treat it as an account. See also CustomAccount.sol which will be used in the PoC.
When L2 transactions are invoked from an account, the bootloader performs an account validation and a transaction payment step. In both cases, the bootloader calls methods of the account which are allowed to perform state changes during those calls. The DefaultAccount does not perform state changes, but a custom account can. Failure at this stage will cause the bootloader to revert (DoS of whole transaction batch).
Once validation and payment are done, the bootloader executes the transaction through the account by calling its executeTransaction(...) method. When a transaction fails, the bootloader does not revert and records the success/failure for finalization. This makes perfect sense and is necessary, otherwise we could only have successful transactions on L2 and never prove failure or see failed transactions in some kind of L2 block explorer. Furthermore, a bootloader revert in case of execution failure would cause DoS for the whole transaction batch.
Since the bootloader does not revert on a failed transaction execution, the state changes that were made during the account validation and transaction payment steps are not reverted. This also makes perfect sense, because the account validation does not require state changes and even a failed transaction needs to be paid for. In case of the DefaultAccount this is harmless, but in case of a custom account all other state changes are preserved too. Furthermore, neither the bootloader nor the Era node can exactly know which transaction caused which state changes, because they are processed in batches.
One can easily create a custom account, that already performs the transaction execution witin its validateTransaction(...) method while purposefully reverting in its executeTransaction(...) method.

As a consequence, we can have "ghost transctions" on zkSync Era where a transaction is executed successfully (state changes and transfer of funds during validation step) with all changes being preserved without logs while still being finalized as failed.
Imagine seeing a failed transaction in some kind of block explorer, but it was actually secretly executed. This could be abused for some pretty elaborate scams.

All in all, this violates a fundamental property of blockchains, since a failed transaction should never lead to any state changes (except for the deduction of transaction fees) and therefore the current vulnerability causes severe inconsistencies.

Proof of Concept

This PoC demonstrates that state changes in the CustomAccount and the CallMe target contract as well as transferred funds are preserved, although the L2 transaction is finalized/confirmed as failed.

Please place the following 2 contracts (linked secret gists) under ./code/system-contracts/contracts/test-contracts:

CustomAccount.sol: based on DefaultAccount.sol, deviations are tagged with @audit-info; performs actual execution during validation; reverts on execution; increments state variable for PoC
CallMe.sol: poses as transction target; stores caller; can receive funds

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

diff --git a/code/system-contracts/test/DefaultAccount.spec.ts b/code/system-contracts/test/DefaultAccount.spec.ts
index 6231c34..d787334 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,
+    CallMe,
+    CustomAccount
 } 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';
@@ -29,6 +31,8 @@ describe('DefaultAccount tests', function () {
     let nonceHolder: NonceHolder;
     let l2EthToken: L2EthToken;
     let callable: Callable;
+    let callMe: CallMe;
+    let customAccount: CustomAccount;
     let mockERC20Approve: MockERC20Approve;
     let paymasterFlowInterface: ethers.utils.Interface;

@@ -48,18 +52,23 @@ 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]
-        });
+        callMe = (await deployContract(`CallMe`)) as CallMe;
+        customAccount = (await deployContract(`CustomAccount`)) as CustomAccount;
+        // fund CustomAccount to cover future EIP-712 transaction fees
+        await wallet.sendTransaction({to: customAccount.address, value: ethers.utils.parseEther("1")});
+        
+        // await network.provider.request({
+        //     method: 'hardhat_impersonateAccount',
+        //     params: [BOOTLOADER_FORMAL_ADDRESS]
+        // });
         bootloader = await ethers.getSigner(BOOTLOADER_FORMAL_ADDRESS);
     });

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

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

     describe('executeTransaction', function () {
+        it.only('successfully executed and state changed, although finalized as failed', async () => {
+            const call = {
+                type: 113,
+                from: wallet.address,
+                to: callMe.address,
+                data: callMe.interface.encodeFunctionData('callMe'),
+                value: ethers.utils.parseEther("0.5")
+            };
+
+            // EIP-712 transaction via CustomAccount abstraction
+            const txRequest = await wallet.populateTransaction(call);
+            txRequest.from = customAccount.address; // use manually deployed CustomAccount to execute transaction
+            txRequest.nonce = 0; // nonce = 0, because first transaction via CustomAccount after deployment
+            txRequest.customData = {
+                ...txRequest.customData,
+                customSignature: "0x1234", // custom signature, see CustomAccount._isValidSignature(...)
+            };
+            const serializedTx = utils.serialize({ ...txRequest });
+
+            // perform transaction
+            const txResponse = await wallet.provider.sendTransaction(serializedTx);
+            
+            // wait for transaction to be finalized
+            try {
+                await txResponse.waitFinalize(); // raises exception on transaction failure
+                throw new Error("should never be reached when transaction is marked as failed");
+            }
+            catch(error) {
+                // transaction failure can be perfectly seen in console output of Era node
+                expect(error.reason).to.be.equal('transaction failed');
+                expect(error.receipt.confirmations).to.be.equal(1);
+            }
+            
+            // *** STATE CHANGED ALTHOUGH TRANSACTION FAILED ***
+
+            // custom account state var is increased on verification and fee payment
+            expect(await customAccount.stateVar()).to.be.equal(2);
+            // target contract should have been called by custom account
+            expect(await callMe.caller()).to.be.equal(customAccount.address);
+            // target contact should have received funds from custom account
+            expect(await l2EthToken.balanceOf(callMe.address)).to.be.equal(call.value);
+            
+        });
+
         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 no state changes can be made at this point. The DefaultAccount needs to be adapted accordingly.
Since the payment step could involve complex operations via a paymaster where tokens need to be converted to ETH, a staticcall is not feasible here. I suggest, that the payment and execution steps are placed into one near call frame in the bootloader and use nearCallPanic() to revert those 2 steps on failure. Afterwards, the bootloader could try to deduct the transaction fee directly from the account/paymaster whithout calling one of the custom account's methods again.

Assessed type

Other

c4-pre-sort commented 8 months ago

bytes032 marked the issue as primary issue

c4-pre-sort commented 8 months ago

141345 marked the issue as sufficient quality report

miladpiri commented 8 months ago

Invalid. Operator should limit AA validation in gas. Other concerns like that which makes some state changes in validation doesn’t really affect anything rather than infra.

c4-sponsor commented 8 months ago

miladpiri (sponsor) disputed

GalloDaSballo commented 7 months ago

@miladpiri this seems to have validity in:

The validation is basically being used to get free tx processing
The execution reverts, creating an inconsistent state

This could arguably be attributed to the fact that validation is non-view / has side effects

Are you disputing because the account is still paying for costs even if their tx is marked as failed?

GalloDaSballo commented 7 months ago

I think after thinking about this further, the finding is mostly about the semantics of a failed and a succesful transaction

The tx is not a reverted tx, it's simply not a succesful one, meaning that:

Gas cost was paid
State was changed

I'd like to hear both sides for the sponsor @miladpiri and the Warden

GalloDaSballo commented 7 months ago

As of now I'd have to downgrade to QA, but will refrain from doing so, will send to PJQA with a note that I'd like to hear both sides

miladpiri commented 7 months ago

I see this report as duplicate of https://github.com/code-423n4/2023-10-zksync-findings/issues/93 Both discussing about execution (state change) during validation.

c4-judge commented 7 months ago

GalloDaSballo marked the issue as selected for report

MarioPoneder commented 7 months ago

Hi @GalloDaSballo,
I agree with both of your points:

Gas cost was paid

One can only escape the gas fee payment by reverting the bootloader during the account's validation or payment step which would in turn revert all the state changes.
This is a separate vulnerability, that is useful for escaping the gas fee on legitimately failed transactions, and is covered by #520.

State was changed

It's worth to add that those state changes are not limited to the affected account but could also include state changes in external contracts/accounts and DeFi protocols if they were interacted with. Like in a normal successful transaction.
This is unexpected and breaks users' trust if such state changes are not reverted on a transaction which is shown as failed.

Let's continue by answering the sponsor's comments and elaborating further on the impacts:

Sponsor comment 1:

Operator should limit AA validation in gas. Other concerns like that which makes some state changes in validation doesn’t really affect anything rather than infra.

The dispute reason seems like a good mitigation measure, but it's insufficient to mitigate the present vulnerability due to the following reasons (where each reason keeps the vulnerability open on its own):

The current bootloader in scope does not implement a mechanism that allows the operator to specifically limit gas usage on account validation or payment. Please call me out, if I overlooked it.
The opposite is true, since the bootloader's callAccountMethod(...) function passes all the available gas to the called account method, e.g. validation or payment. Furthermore, the gas spent during account validation & payment is only checked against the overall transaction gas limit, therefore allowing to abuse the vulnerability as initially described in the report.
It is additionally possible to move the execution from the account's validation into the account's payment step, which was also pointed out as a side note in my other issue #520 (just for the record).
The payment step intends to allow more complex logic in order to pay for transaction fees, either through the custom account or using a custom paymaster.
During this step, a specifically small gas limit would never be reasonable since it is an intended use case to swap e.g. a token for ETH in order to pay for the transaction fees during this step.
One can simply re-run the above PoC with this modified CustomAccount.sol to validate that custom execution can also be done within the payment step.
If necessary, the execution could also be done by a custom paymaster, in case the payment step of an account could become subject to specific gas limits too.

Sponsor comment 2:

I see this report as duplicate of #93
Both discussing about execution (state change) during validation.

The present issue is not about a custom account implementation per se, but rather about the bootloader being vulnerable and therefore allowing a malicious custom account to cause the given impacts, therefore not OOS.
Although the mentioned issue seems to identify the core problem, it's lacking on development and discussion of impacts, especially regarding a runnable PoC which shows that state-preserving interactions with external contracts are indeed possible.

Nevertheless, it's fully up to the judge if the mentioned issue eventually gets duplicated to the present one or not.

Impact:
As a consequence of this vulnerability, complex state-modifying contract interactions can be executed within a custom account's validation and/or payment methods. Those are not limited to infra and could be interactions with DeFi protocols for instance. The PoC demonstrates a state-modifying interaction with an external contract.

However, all this can be done while the account's "real" execution step purposefully reverts.
Therefore, the transaction will be shown as failed by the Era Node, by the zkSync Era Block Explorer and by any Web3 provider that queries the node, while its state changes are preserved, effectively disguising the transaction.

Node output - Successful transaction (state changes preserved) shown as FAILED + gas usage:

21:31:50 [INFO] Executing 0x44253570df4ed1888231bef2acc9aa08109db192737d43d0de8018ba2a584994
21:31:50 [INFO] ┌─────────────────────────┐
21:31:50 [INFO] │   TRANSACTION SUMMARY   │
21:31:50 [INFO] └─────────────────────────┘
21:31:50 [INFO] Transaction: FAILED
21:31:50 [INFO] Initiator: 0x094499df5ee555ffc33af07862e43c90e6fee501
21:31:50 [INFO] Payer: 0x094499df5ee555ffc33af07862e43c90e6fee501
21:31:50 [INFO] Gas - Limit: 1_686_370 | Used: 1_477_936 | Refunded: 208_434

Test output:

  DefaultAccount tests
    executeTransaction
      √ successfully executed and state changed, although finalized as failed (92ms)

(Please revisit the existing PoC for more details)

Such a state vs. transaction behaviour should not be possible in any case (EVM compatibility) and opens the gate for scammers.
Malicious actors could e.g. trick victims into thinking a transfer of funds was not successful or try to cover their traces using seemingly failed transactions that still lead to successful state changes.

Two further points of thought:

When reconstructing a chain's state by processing all transactions since genesis, would anyone expect state changes from failed transactions? (except for fee deduction)
As I came across #761, isn't the impact of emitting logs/events from failed/reverted transactions similarly bad as keeping state changes of seemingly failed transactions?

Thanks and have a nice day, everyone!

miladpiri commented 7 months ago

It is intended behavior. The point of AA is that generally validation can include any logic. EIP4337 for instance allows that as well (any limitations on the validation step are purely done offchain and not guaranteed by the protocol). So it is just limited to infra. It is more of a question of how to display such txs in the explorer.

MarioPoneder commented 7 months ago

ERC-4337 itself requires to restrict call/delegateCall/staticcall during the validation step which would make the present vulnerability impossible.
On zkSync Era it's intended to improve UX (https://era.zksync.io/docs/reference/concepts/account-abstraction.html#extending-eip4337) and thereby the present vulnerability is introduced by:

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

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

Alternatively, one could also follow the exploit path through the payment step or a custom paymaster in order to create seemingly failed transactions with preserved state changes. More details are provided in this comment.

Nevertheless, if the sponsor sees this as intended and a won't fix, I don't wanna argue against that. But I hope this does not impact the validity of the submission considering the shown impacts.

Edit: Since it seems to be the consensus in this contest that the operator cannot be trusted, I further recommend to mitigate the present issue within the bootloader, i.e. on-chain, see Recommended Mitigation Steps for my ideas.

5z1punch commented 7 months ago

Provide some facts in the operators code:

There is the specifically limit gas usage on account validation in tracer: https://github.com/matter-labs/zksync-era/blob/c594323054f5243ca08d6db3f59e007f14e30f4f/core/lib/multivm/src/tracers/validator/vm_latest/mod.rs#L39-L43

If the gas cost by validation is greater than this, your tx can be just blocked on the rpc sandbox, even can't enter the main node.

Although the AA of era extends EIP4337, its still subject to strict limitations. The codes here: https://github.com/matter-labs/zksync-era/blob/c594323054f5243ca08d6db3f59e007f14e30f4f/core/lib/multivm/src/tracers/validator/vm_latest/mod.rs#L46-L124

The AA can is allowed to call/delegateCall/staticcall contracts. But it can't read / write any storage doesn't belong to it. And it also can't emit any events or read the context of the current block, such as gasleft or timestamp. Unless, its calling KECCAK256, msgValueSimulator or the bridge token contracts, which is saved in the blockchain database by tracing bridge events.

The paymaster is not subject to these limits. If the paymaster needs to add these restrictions, I'm sure they can just be removed.
The last fact: if the tx fails in the account validation, it will be just blocked on the rpc sandbox, even can't enter the main node.

MarioPoneder commented 7 months ago

Thank you for providing those facts and confirming that the paymaster is not subject to those limitations, which keeps the vulnerability fully open (see my first comment).
Nevertheless, it's worth to mention that in spite of those limitations that should be in place, the PoC shows that another contract's storage can be written during the validation step.

The last fact: if the tx fails in the account validation, it will be just blocked on the rpc sandbox, even can't enter the main node.

Also agree with this one. However, the validation/payment step is meant to succeed in order to create "ghost transactions".

GalloDaSballo commented 7 months ago

While I have made a sizeable effort to find scenarios in which a sidestep, or a broken invariant would happen

I believe that the finding is QA as these are sideeffects of having custom code to manage an account

The sandboxing shown by further comments shows how the account could be used to store extra data, e.g. some oracle data, while the main goal would be storing nonces

This is important and worth flagging

However, that data would be paid for by processing gas costs

And that data would be available only to the custom account, which ultimately defeats the purpose of storing it on chain in the first place

I also considered the return vaule of the bootloader indicating a false success while state changes happened but I also believe that:

Those operations will be part of the L2
Those will be limited to internal settings for the account, which is in line with how nonce and reverts works for an account

Due to this, I believe that the best categorization for the finding is QA and invite the warden to investigate further ways to escape the sandbox which may warrant a higher severity

c4-judge commented 7 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)

MarioPoneder commented 7 months ago

Hi @GalloDaSballo, thank you for inviting me to elaborate further on escaping the sandbox, since I feel like we are still talking past each other on this particular issue as well as #520.

In addition to my main comments (#1 and #2), the PoC of the original submission itself proves that the sandbox is escaped:

Modifying a state variable of the CallMe contract and transferring funds to it from within the validation step of a custom account.
This CallMe contract is totally unrelated to the custom account and could also be a DeFi protocol's contract.
Letting the "real" execution step of the custom account revert to flag the transaction as failed.
Wait until the failed transaction is finalized.
Afterwards, check that the state change of the CallMe contract and transferred funds are still there --> success. This state/data is available to anyone.

This runnable PoC indisputably shows that state changes / fund transfers are preserved on "failed" transactions affecting unrelated contracts, not only the custom account itself. This was not prevented by restrictions or sandboxing and we have not seen any proof yet that invalidates this runnable PoC.
In the end, we have the state and balance of an unrelated contract modified by a failed transaction.

Or in other words: Sandboxing is not an issue because after the sandboxed validation step succeeds, it's still executed "for real" by the bootloader on-chain.

Throughout the comments, it became evident that the validation step is subject to restrictions while it was confirmed that such restrictions do not apply during the payment step, therefore I supplied an addition to my PoC in the first comment where the above scenario is also proven for the payment step.
If requested, I could also supply another PoC where the above scenario is done via a custom paymaster.

I really appreciate the judge digging into this and I could go on reiterating previous facts from another point of view. However, I feel like another wall of text from my side will only lead to misunderstanding each other further. Therefore I respectfully point to my main comments (#1 and #2) since, from my point of view, they sufficiently discuss impacts, PoC via payment step and ineffectiveness of restrictions.

Thank you!

Edit: Summary:

State and balance changes of other contracts (not limited to account) are preserved on-chain (not in sandbox) and accessible by anyone
Although transaction shown as failed by Web3 provider, Era Node log and zkSync Block Explorer --> Inconsistencies, EVM equivalence?
Restrictions that might lead sandboxed validation step to failure can be circumvented via payment step or even custom paymaster
Current restrictions in validation step have intended loophole that allows calls to other contracts --> circumvents state/data access restrictions
PoC of initial submission is runnable and was not disproved

Edit 2: On Ethereum the present issue would not be possible, since EIP-4337 disallows access to any information that could introduce differences between sandboxing and actual execution during the validation step.

Assume 2 accounts A & B modify the state of contract C during validation:

Sandbox passes because A & B work with the same original/unmodified state of C.
Execution of A modifies the state of C on-chain.
Execution of B fails, because state of C differs from sandbox. --> bootloader revert / DoS

From EIP-4337:

In addition, a UserOperation is only allowed to access data related to this sender address: Multiple UserOperations should not access the same storage, so that it is impossible to invalidate a large number of UserOperations with a single state change.

This scenario would be possible on zkSync Era due to the present vulnerability.

vladbochok commented 7 months ago

Hey @MarioPoneder,

I want to clarify a bit on the AA design:

The current bootloader in scope does not implement a mechanism that allows the operator to specifically limit gas usage on account validation or payment. Please call me out, if I overlooked it.

Operator can only include the transaction that went through the validation and payment step, it is enforced by the bootloader implementation. When operator creates a batch it runs validation of the transaction for X amount of gas. If more gas is used then operator just reject the transaction. So, the way how operator includes the transaction in the batch is the protection here.

As a consequence of this vulnerability, complex state-modifying contract interactions can be executed within a custom account's validation and/or payment methods. Those are not limited to infra and could be interactions with DeFi protocols for instance. The PoC demonstrates a state-modifying interaction with an external contract.

This is how AA works, yes, account can make state changes in the validation step while the execution step failed, all the storage changes will be shown on the block explorer properly (if not it is infra problem as was mentioned before).

Although transaction shown as failed by Web3 provider, Era Node log and zkSync Block Explorer --> Inconsistencies, EVM equivalence?

AA is an add-on feature, so all custom accounts may have specific/new behaviour. This is not EVM equivalence inconsistency since all other contracts behave exactly the same as on EVM, while only AA users/devs need to understand the new behaviour.

Lastly, thanks for having the great thread here for both warden and judge. But with all due respect I can't see the security issue here. State changes can happen in both validation/payment and execution step, failed transaction may interact with other contracts and its intended behaviour that doesn't break any specific EVM in-execution invariants. I feel this is QA as for lack of impact.

GalloDaSballo commented 7 months ago

I would like more information on specifically why / how the sandbox is avoided, afaict only "trusted" slots can be touched, and those would be empty with the exception of the slots of the AA contract

https://github.com/matter-labs/zksync-era/blob/c594323054f5243ca08d6db3f59e007f14e30f4f/core/lib/multivm/src/tracers/validator/vm_latest/mod.rs#L46-L124

Specifically want details here, as my counterpoint to the POC is that it's not using the prod setup so validation that the rust code would do is ignored

MarioPoneder commented 7 months ago

Hi @GalloDaSballo,
thank you for pointing this out. I was assuming that the Era Test Node is also running the check_user_restrictions_vm_latest function, since I could not find any indication that it's skipped. Also there seems to be no restriction of storage writes, only storage reads. Therefore, I'd need a production test setup to verify if my initial PoC would fail there (any help is appreciated).
However, I now agree that arbitrary transactions within the account's validation/payment step theoretically should not work, because the check_user_restrictions_vm_latest function is invoked on UserTxValidation which includes an account's validation/payment step according to the bootloader hooks #1 and #2.

However, those restrictions do not apply when using a custom paymaster, i.e. PaymasterTxValidation:

For now, we support only validations for users.

See also the corresponding bootloader hooks.

As already mentioned in earlier comments, the described impacts (full execution of transaction while transaction shown as failed, i.e. state/balance changes of unrelated contracts) can be realized via a custom paymaster and thereby circumventing the sandbox.
This is possible because also the paymaster gets the full transaction data.

Please give me 12h from now to provide a PoC for the paymaster case, otherwise please let me know if you don't want/need a further PoC.

Thanks everyone for having this interesting discussion.

Edit:
The PoC is done! Please follow the PoC steps of the initial submission, just use this CustomAccount.sol which also implements IPaymaster and is therefore its own paymaster. Here the transaction is executed within validateAndPayForPaymasterTransaction(...) (implemented at the bottom of CustomAccount.sol) where no sandbox restrictions apply, see discussion above.
Furthermore, please include the paymaster info in the EIP-712 transaction of the initial test case:

txRequest.customData = {
    ...txRequest.customData,
    customSignature: "0x1234", // custom signature, see CustomAccount._isValidSignature(...)
    paymasterParams: {
        paymaster: customAccount.address,
        paymasterInput: "0x8c5a3445" // IPaymasterFlow.general.selector
    }
};

With those changes, the PoC succeeds again and would not be subject to operator/sandbox restrictions in production.

Please note that no separate paymaster contract is needed here. It's still just a custom account where the payment and additional validation is handled via its paymaster methods and thereby fully avoiding the sandbox restrictions. Consequently, complex state-modifying interactions with other contracts can take place here while still showing the transaction as failed.

Edit 2: @GalloDaSballo
In the mean time I thought a lot about how to mitigate this one, since it's quite easy to circumvent the account's validation/payment sandbox restrictions by making it its own paymaster.
Furthermore, the paymaster is intended to make some kind of DeFi transactions, therefore one cannot impose meaningful restrictions there.
However, the bootloader could at least enforce that the paymaster and custom account are not the same contract, or enforce that the paymaster is not an account.
This way an arbitrary "ghost" transaction could not have msg.sender = address(customAccount), but only msg.sender = address(paymaster) which is not the same anymore as executing the transaction through the account.

vladbochok commented 6 months ago

Hey @MarioPoneder, appreciate the effort you put into this report!

The whole discussion is around the fact that failed transactions may make state changes in the AA model. It is true and there is nothing wrong. I don't see any specific impact here in the thread rather than infrastructure can show transaction data better. Regarding EIP-4337, since the entry point is a publicly available smart contract - anyone can make AA transaction that bypasses safety DoS restrictions, so even on Ethereum failed AA transaction may actually change the state.

This way an arbitrary "ghost" transaction could not have msg.sender = address(customAccount), but only msg.sender = address(paymaster) which is not the same anymore as executing the transaction through the account.

I don't see any benefits of having the check, since there are many other edge cases to the problem with no clear impact. Example of such edge case customAccount1 uses customAccount2 as a paymaster.

c4-judge commented 6 months ago

GalloDaSballo marked the issue as not selected for report

GalloDaSballo commented 6 months ago

After a long discussion above as well as discussions with the sponsor I believe I can't justify a bump to Medium Severity

AA inherently alters some fundamental behaviour of how TXs are handled in the EVM

This finding is very insightful if we assume the invariant that state changes can only happen when the Bootloader returns a success for a transaction

Due to Native AA this is not the case

As AA effectively has a "pre-turn" in which it can be used to perform state changes, the simplest of which is nonce changes

While it is arguable as to what AA should be able to access to, we can agree that for validation purpose it may be necessary to retrieve values for nonces as well as funds

This lead the Sponsor to allow AA accounts to alter state in a way that could be considered unintended

From another perspective, the AA has the ability to alter any state, since it's paying for it's gas and its still abiding by all rules of the EVM, to that end the AA "turn" is not inherently dangerous but more so a gotcha

From an architectural POV txs with native AA end up behaving in the following way

[ AA Success, Bootloader Success | Failure ]

[ AA Failure ]

Meaning that an explorer can misflag a state-changing transaction as reverted if it was using the bootloader pointer as the only source of information

However, any tx in which the AA "turn" is succesful may have state-changes, and as such displaying the tx as reverted would be incorrect, however this wouldn't break any specific invariant but rather demonstrate an issue with how the system is represented to end users.

In conclusion, due to the information available, I am not convinced I can make the case to raise the finding to Medium Severity, and due to that, I'm keeping it as Low Severity

code-423n4 / 2023-10-zksync-findings

State changes are preserved on failed L2 transactions using custom account abstraction #469

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type