c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

The EIP-3607: Reject transactions from senders with deployed code requires transactions where the from address (tx.origin in Solidity) is not an EOA to be rejected.
This EIP was alread merged into the Ethereum yellow paper and the Geth execution client.

However, in case of EIP-712 transactions on L2 using custom account abstraction, the from address is not an EOA due to having a codehash, therefore the bootloader uses its own address as tx.origin, see ZKSYNC_NEAR_CALL_executeL2Tx(...):

function ZKSYNC_NEAR_CALL_executeL2Tx(
    abi,
    txDataOffset
) -> success {
    // Skipping the first word of the ABI-encoding encoding
    let innerTxDataOffset := add(txDataOffset, 32)
    let from := getFrom(innerTxDataOffset)

    debugLog("Executing L2 tx", 0)
    // The tx.origin can only be an EOA
    switch isEOA(from)
    case true {
        setTxOrigin(from)
    }  
    default {
        setTxOrigin(BOOTLOADER_FORMAL_ADDR())
    }

    success := executeL2Tx(txDataOffset, from)
    debugLog("Executing L2 ret", success)
}

The bug is, that the bootloader address also points to a non-zero & non-empty codehash, see also AccountCodeStorage.getRawCodeHash(...) & AccountCodeStorage.getCodeHash(...), therefore the bootloader is not recognized as an EOA which defies the purpose of the above switch-case block.

As a consequence, EIP-712 transactions using custom account abstraction do not comply with EIP-3607 and therefore are in danger to fail:

Contracts could enforce their own EIP-3607 check.
The Era test node does not seems to enforce EIP-3607 yet, but this should be different in production to reach best compatibility/conformity with the L1 EVM.

Side note: Strictly, account validation and (paymaster) gas refund of any transaction type are also not compliant with EIP-3607, due to tx.origin being the bootloader address, but those are special cases and not responsible for the execution of the actual transaction.

The main issue remains that Legacy & EIP-1559 L2 transactions comply with EIP-3607, but EIP-712 transactions using custom account abstraction do not.

Proof of Concept

The following PoC tests the EIP-3607 conformity of multiple transaction types and shows that the tx.origin, in case of EIP-712 transactions using custom account abstraction, indeed points to non-zero & non-empty codehash leading to nonconformity.

First, add the custom account contract ./code/system-contracts/contracts/test-contracts/CustomAccount.sol from the secret gist (too long to include in report). It is an 1:1 copy of the DefaultAccount, just with custom signature validation and it registers itself as account on construction.

Second, add the checker test contract ./code/system-contracts/contracts/test-contracts/EIP3607Checker.sol:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8;

import {ACCOUNT_CODE_STORAGE_SYSTEM_CONTRACT} from "../Constants.sol";

contract EIP3607Checker {
    // see AccountCodeStorage.sol:L29
    bytes32 constant EMPTY_STRING_KECCAK = 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470;

    bool public compliant;

    function check() external returns(bool) {
        bytes32 originCodeHash = ACCOUNT_CODE_STORAGE_SYSTEM_CONTRACT.getCodeHash(uint256(uint160(tx.origin)));
        compliant = originCodeHash == EMPTY_STRING_KECCAK || originCodeHash == 0x00;
    }
}

Next, 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..7fb0242 100644
--- a/code/system-contracts/test/DefaultAccount.spec.ts
+++ b/code/system-contracts/test/DefaultAccount.spec.ts
@@ -7,19 +7,23 @@ import {
     Callable,
     L2EthToken,
     L2EthToken__factory,
-    MockERC20Approve
+    MockERC20Approve,
+    EIP3607Checker,
+    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';
 import * as zksync from 'zksync-web3';
 import { TransactionData, signedTxToTransactionData } from './shared/transactions';
+import exp from 'constants';
+import { assert } from 'console';

 describe('DefaultAccount tests', function () {
     let wallet: Wallet;
@@ -29,6 +33,8 @@ describe('DefaultAccount tests', function () {
     let nonceHolder: NonceHolder;
     let l2EthToken: L2EthToken;
     let callable: Callable;
+    let eip3607Checker: EIP3607Checker;
+    let customAccount: CustomAccount;
     let mockERC20Approve: MockERC20Approve;
     let paymasterFlowInterface: ethers.utils.Interface;

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

+        eip3607Checker = (await deployContract(`EIP3607Checker`)) as EIP3607Checker;
+        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]
@@ -152,6 +163,64 @@ describe('DefaultAccount tests', function () {
     });

     describe('executeTransaction', function () {
+        it.only('EIP3607Checker run', async () => {
+            let call = {
+                type: 0,
+                from: wallet.address,
+                to: eip3607Checker.address,
+                data: eip3607Checker.interface.encodeFunctionData('check'),
+            };
+
+            // Legacy transaction
+            call.type = 0;
+            let tx = await wallet.populateTransaction(call);
+            assert(tx.type == 0);
+            await wallet.sendTransaction(tx);
+            // Check if EIP-3607 compliant
+            expect(await eip3607Checker.compliant()).to.be.equal(true);
+
+
+            // EIP-1559 transaction
+            call.type = 2;
+            tx = await wallet.populateTransaction(call);
+            assert(tx.type == 2);
+            await wallet.sendTransaction(tx);
+            // Check if EIP-3607 compliant
+            expect(await eip3607Checker.compliant()).to.be.equal(true);
+
+
+            // EIP-712 transaction via DefaultAccount abstraction
+            call.type = 113;
+            tx = await wallet.populateTransaction(call);
+            assert(tx.type == 113);
+            const txData = signedTxToTransactionData(utils.parseTransaction(await wallet.signTransaction(tx)))!;
+            tx.from = wallet.address; // use zksync DefaultAccount to execute transaction
+            tx.customData = {
+                ...tx.customData,
+                customSignature: txData.signature,
+            };
+            let serializedTx = utils.serialize({ ...tx });
+            await wallet.provider.sendTransaction(serializedTx);
+            // Check if EIP-3607 compliant
+            expect(await eip3607Checker.compliant()).to.be.equal(true); 
+
+
+            // EIP-712 transaction via CustomAccount abstraction
+            call.type = 113;
+            tx = await wallet.populateTransaction(call);
+            assert(tx.type == 113);
+            tx.from = customAccount.address; // use manually deployed CustomAccount to execute transaction
+            tx.nonce = 0; // nonce = 0, because first transaction via CustomAccount after deployment
+            tx.customData = {
+                ...tx.customData,
+                customSignature: "0x1234", // custom signature, see CustomAccount.sol:L169
+            };
+            serializedTx = utils.serialize({ ...tx });
+            await wallet.provider.sendTransaction(serializedTx);
+            // Check if EIP-3607 compliant
+            expect(await eip3607Checker.compliant()).to.be.equal(false); // NOT compliant!!!
+        });
+
         it('non-deployer ignored', async () => {
             let nonce = await nonceHolder.getMinNonce(account.address);
             const legacyTx = await account.populateTransaction({

Tools Used

Manual review

Recommended Mitigation Steps

As suggested by EIP-2938: Account Abstraction, use 0xffffffffffffffffffffffffffffffffffffffff instead of the bootloader's address as tx.origin on EIP-712 transactions to be compliant with EIP-3607 and consistent with the L1 EVM.

Assessed type

Context

c4-pre-sort commented 11 months ago

bytes032 marked the issue as primary issue

c4-pre-sort commented 11 months ago

bytes032 marked the issue as sufficient quality report

miladpiri commented 11 months ago

Invalid. Due to the nature of native AA, we ignore EIP3607.

c4-sponsor commented 11 months ago

miladpiri (sponsor) disputed

GalloDaSballo commented 10 months ago

@miladpiri wdyt about preventing a EOA being able to take ownership of a SC? Wouldn't it be better to enforce the check?

miladpiri commented 10 months ago

Regarding the report:

If from is an EOA (default account), then tx.origin will be equal to from.
If from is a custom account, then it's raw-code-hash will be nonzero, so it will not be considered as an EOA. So, tx.origin will be set to bootloader address, and since extcodehash(bootloader) returns nonzero and none-EMPTY_STRING_KECCAK, tx.origin will not be considered as an EOA anymore. This is expected due to nature of native AA.

So, this finding is not an issue or bug.

Regarding your question @GalloDaSballo , I could not understand it correclty.

GalloDaSballo commented 10 months ago

Regarding the report:

If from is an EOA (default account), then tx.origin will be equal to from.

If from is a custom account, then it's raw-code-hash will be nonzero, so it will not be considered as an EOA. So, tx.origin will be set to bootloader address, and since extcodehash(bootloader) returns nonzero and none-EMPTY_STRING_KECCAK, tx.origin will not be considered as an EOA anymore. This is expected due to nature of native AA.

So, this finding is not an issue or bug.

Regarding your question @GalloDaSballo , I could not understand it correclty.

EIP-3607 was added to prevent the scenario of being able to find the PK of a contract that contains value, without it any PK that maps out to an address could start a tx as an EOA and steal the funds

Basically the check prevents being able to find a PK that maps out to a deployed Smart Contract

From the EIP

We estimate that a collision between a contract and an EOA could be found in about one year with an investment of ca. US$10 billion in hardware and electricity.

Meaning that this could be weaponized in the future

Is EIP-3607 already enforced on zkSync? Do you think it should / should not?

miladpiri commented 10 months ago

We already prevent the attack mentioned by:

Enforcing that type 0, 1, 2 transactions can only be started by an EOA:

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

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

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

For type 712 transactions, the account from is called, i.e. just EOA address collision is not enough. The account must literally accept the signature, which at this case, it is not a bug anymore. In other words, when an EOA triggers a transaction, the default account contract is called by the bootloader to validate the signature. In case, if a PK of a contract is found, it is still not possible to drain the fund. Because that contract must implement the validateTransaction function and validate the signature.

c4-judge commented 10 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 10 months ago

Downgrading to QA and invite the Warden to send a follow up in PJQA, I believe that zkSync has addressed the underlying issue as shown above by the Sponsor

MarioPoneder commented 10 months ago

Hi @GalloDaSballo,
Following up on the present issue:

Feedback: Thanks for defending the report and digging deeper on this topic. This is a good example of how to act as a judge.
I agree with the sponsor @miladpiri:

Due to the nature of native AA, we ignore EIP3607.

In case someone got the private key for an address where a contract is deployed and sends a transaction from that address, the bootloader's ensureAccount(...) function would revert at the beginning of the validation step even before any signature checks are performed.
If this check passes because the contract was registered as an account at the ContractDeployer, the contract would still need to implement the validateTransaction(...) (as mentioned by the sponsor), executeTransaction(...) and payForTransaction(...) methods in order for the transaction to be executed.
Due to this native implementation of account abstraction, the attack outlined by EIP-3607 is not possible on zkSync Era, therefore they chose to ignore EIP-3607 compliance as a whole.
The main concern of the report is about tx.origin (bootloader address) and its codehash not being an EOA, therefore EIP-712 transactions are not compliant with EIP-3607 and not consistent with the L1 EVM.
- If the Era node implements an EIP-3607 check, all such transactions would fail.
  We now know that EIP-3607 won't be implemented on zkSync Era.
- If a contract implements an EIP-3607 check of tx.origin (very unlikely, but possible), such transactions would fail.

All in all, zkSync Era strives for equivalence with the EVM where possible to avoid compatibility issues for protocols running on multiple chains including zkSync Era.
Following the Recommended Mitigation Steps, i.e. complying with EIP-2938: Account Abstraction concerning tx.origin, native account abstraction and EIP-3607 could go hand-in-hand.

miladpiri commented 10 months ago

Thanks @MarioPoneder

I do not see any issue. Moreover, while compiling a contract that is using tx.origin in zkSync Era, a warning is displayed: https://github.com/matter-labs/era-compiler-solidity/blob/main/src/solc/standard_json/output/error/mod.rs#L123

GalloDaSballo commented 10 months ago

I believe that this finding was a good addition to the contest and a worthwhile discussion

This ultimately highlights a gotcha due to native account abstraction

Overall the finding points at a risk that integrators can have

For this reason, I believe that QA is most appropriate for this contest

The responsibility of defending their code will fall to the integrators, for those contracts this finding may cause further impacts, however that would require said contracts to be in-scope which in this case they are not

For this reason, after confirming with the Sponsor that EOAs can start txs, but also agreeing with the Warden that tx.origin checks can create gotchas, am going to maintain QA severity

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

EIP-712 transactions via custom accounts do not comply with EIP-3607 and could therefore fail #322

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type