Biconomy - Smart Contract Wallet contest - aviggiano's results

Lines of code

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

Vulnerability details

Impact

SmartAccountFactory.deployCounterFactualWallet can be frontrun, funds can be stolen and a backdoor can be introduced on SmartAccount.

Since the deployment of a counterfactual wallet only depends on the _owner address and on the _index of the wallet for the same EOA, an attacker can examine the mempool and frontrun the wallet creation, introducing malicious code to gain access to the deployed SmartAccount.

The malicious code can be introduced either through the user-provided _entryPoint or _handler function parameters:

1. _entryPoint: for example by sending the own attacker address, which will later be used to call execFromEntryPoint and execute arbitrary code (as the onlyEntryPoint modifier will pass). It is possible to perform the following actions to gain express ownership: execFromEntryPoint -> will call execute -> will call _call -> will call setOwner -> mixedAuth modifier will pass
2. _handler: for example by setting an attacker-controlled malicious contract, which will be used as fallback handler and will be used to execute arbitrary code. It is possible to perform the following actions to gain express ownership: fallback -> will call execute -> will call _call -> will call setOwner -> mixedAuth modifier will pass. Note: SmartAccountFactory.deployWallet is also vulnerable to this attack vector.

After the attacker has gained access to the _owner wallet, it should perform its malicious actions knowing that the transaction that was frontrun will subsequently attempt to CREATE2 the contract with the same address, which will revert in case it exists. This can be circumvented by using SELFDESTRUCT, so that the next transaction does not revert. It is thus possible to use different attack vectors to steal user funds, with varying degrees of risk (likelihood/impact), described in decreasing order of severity in the "Proof of Concept" section.

Proof of Concept

P1. Attacker can introduce a backdoor by fruntruning -> approving ERC-20 tokens -> selfdestruct

Similar to the attack vector described on Damn Vulnerable Defi's Backdoor challenge, it is possible to introduce a backdoor on contracts that where an exploiter has limited-time access, such as the case in this attack. More specifically, after gaining access to the SmartAccount, the attacker can approve any ERC-20 token (or ERC-721/ERC-1155) to another attacker-controlled account, and later SELFDESTRUCT so that the subsequent transaction does not revert. As a consequence, it can steal all funds at a later step. Step by step:

1. Alice, an account wishing to create a SmartAccount for Bob, submits a transaction with SmartAccountFactory.deployCounterFactualWallet
2. Eve, a malicious attacker, watches the mempool and submits a transaction with a higher priority, and frontruns Alice's transaction. 2a. Eve gains control of Bob's SmartAccount using one of the methods described in the previous section 2b. Eve approves ERC-20 tokens from Bob's SmartAccount to Eve's account 2c. Eve selfdestructs Bob's SmartAccount contract and the transaction ends
3. Alice's transaction is included in the block and does not revert. As a result, Bob's SmartAccount is successfully created with a backdoor included, unbeknownst to either Alice or Bob
4. Eve can now steal any of Bob's ERC-20 tokens.

Since this attack has no preconditions (all wallet creations are vulnerable to this) and a high impact (hard-to-detect backdoor capable of stealing all present and future user funds), it is a highly critical attack.

A full test of this attack is available here. In order to verify it, download the patch file, run git apply backdoor.patch and npx hardhat test.

P2. Attacker can steal previously sent funds to the counterfactual address by frontrunning

Knowing that one of Biconomy's Smart Account creation flow is users paying for the wallet creation by sending funds to the counterfactual address before it is deployed (see "B. Users Pays The Wallet Creation Cost"), an attacker can frontrun the transaction and steal any funds that had been sent to the contract.

1. Alice, an account wishing to create a SmartAccount for Bob, submits a transaction with SmartAccountFactory.deployCounterFactualWallet
2. Eve, a malicious attacker, watches the mempool and submits a transaction with a higher priority, and frontruns Alice's transaction. 2a. Eve gains control of Bob's SmartAccount using one of the methods described in the previous section 2b. Eve steal funds that had been sent to the counterfactual address

Since this attack has a precondition (previously sent funds), and a smaller impact (only funds necessary to wallet creation, typically only ETH for gas fees), it is less critical than P1.

P3. Attacker can forge SmartAccount signatures possibly gaining access to third-party applications

Since one of SmartAccount's features is EIP-1271 contract signatures, a feature inspired by the one from Gnosis Safe, it is possible that the attacker-controlled SmartAccount can forge its signature and possibly gain access to third-party applications, assuming these applications rely on SmartAccount's signatures for a verification scheme.

1. Alice creates a multisig (this can be a Gnosis Safe or a Biconomy multisig wallet)
2. Alice includes Bob's SmartAccount address as one of the signers, as this is a counterfactual address. Since Bob's account is a contract, the multisig will expect an EIP-1271 signature
3. Eve, an attacker, frontruns Bob's account creation, and executes a transaction on Alice's wallet, with a valid EIP-1271 signature, giving control of the multisig to Eve
4. Eve steals all the funds on the multisig

Since this attack has a precondition (third-party contracts trusting counterfactual addresses before they are created), and a high impact (funds from third-party applications), it is less critical than P1.

Tools Used

Manual review

Recommended Mitigation Steps

R1. Remove the _entryPoint parameter from the function, as this is a highly trusted address that, if wrongly provided, maliciously or unintentionally, can cause serious damage to the SmartAccount contract. This should be a state variable from the SmartAccountFactory contract. In case the global entrypoint needs to be updated, the own SmartAccount has a updateEntryPoint method to do it. Note that this alone does not stop the attack, as _handler is still a user-defined parameter. R2. Verify that a wallet has not been created through the isAccountExist variable before proceeding with create2. This will alert users that their account creation has been frontrun, as the transaction will revert. Note that this still enables an attacker to introduce a backdoor to the user's account, so they must be aware not to use accounts in case SmartAccountFactory.deployCounterFactualWallet reverts. R3. Remove the _handler parameter from the wallet creation and let the own SmartAccount include a fallback handler later through execute, as non-owners creating other users' wallets can introduce a backdoor through _handler.

All suggestions considered:

diff --git a/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol
index d1698f7..ab6703e 100644
--- a/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol
@@ -111,7 +111,7 @@ abstract contract BaseSmartAccount is IAccount {
         }
     }
     
-    function init(address _owner, address _entryPointAddress, address _handler) external virtual;
+    function init(address _owner, address _entryPointAddress) external virtual;
 
     function execTransaction(
         Transaction memory _tx,
diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
index c4f69a8..1f143a7 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
@@ -163,15 +163,13 @@ contract SmartAccount is
     // Initialize / Setup
     // Used to setup
     // i. owner ii. entry point address iii. handler
-    function init(address _owner, address _entryPointAddress, address _handler) public override initializer { 
+    function init(address _owner, address _entryPointAddress) public override initializer { 
         require(owner == address(0), "Already initialized");
         require(address(_entryPoint) == address(0), "Already initialized");
         require(_owner != address(0),"Invalid owner");
         require(_entryPointAddress != address(0), "Invalid Entrypoint");
-        require(_handler != address(0), "Invalid Entrypoint");
         owner = _owner;
         _entryPoint =  IEntryPoint(payable(_entryPointAddress));
-        if (_handler != address(0)) internalSetFallbackHandler(_handler);
         setupModules(address(0), bytes(""));
     }
 
diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol
index be78c75..594968b 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,10 @@ 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");
         _defaultImpl = _baseImpl;
+        _defaultEntryPoint = _entryPoint;
     }
 
     // event SmartAccountCreated(address indexed _proxy, address indexed _implementation, address indexed _owner);
@@ -26,11 +28,10 @@ contract SmartAccountFactory {
     /**
      * @notice Deploys wallet using create2 and points it to _defaultImpl
      * @param _owner EOA signatory of the wallet
-     * @param _entryPoint AA 4337 entry point address
-     * @param _handler fallback handler address
      * @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){
+    function deployCounterFactualWallet(address _owner, uint _index) public returns(address proxy){
+        require(isAccountExist[getAddressForCounterfactualWallet(_owner, _index)] == false, "wallet already exist");
         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
@@ -40,7 +41,7 @@ contract SmartAccountFactory {
         require(address(proxy) != address(0), "Create2 call failed");
         // EOA + Version tracking
         emit SmartAccountCreated(proxy,_defaultImpl,_owner, VERSION, _index);
-        BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
+        BaseSmartAccount(proxy).init(_owner, _defaultEntryPoint);
         isAccountExist[proxy] = true;
     }
 
@@ -48,15 +49,14 @@ contract SmartAccountFactory {
      * @notice Deploys wallet using create and points it to _defaultImpl
      * @param _owner EOA signatory of the wallet
      * @param _entryPoint AA 4337 entry point address
-     * @param _handler fallback handler address
     */ 
-    function deployWallet(address _owner, address _entryPoint, address _handler) public returns(address proxy){ 
+    function deployWallet(address _owner, address _entryPoint) public returns(address proxy){ 
         bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
         // solhint-disable-next-line no-inline-assembly
         assembly {
             proxy := create(0x0, add(0x20, deploymentData), mload(deploymentData))
         }
-        BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
+        BaseSmartAccount(proxy).init(_owner, _entryPoint);
         isAccountExist[proxy] = true;
     }
 
@@ -65,7 +65,7 @@ contract SmartAccountFactory {
      * @param _owner EOA signatory of the wallet
      * @param _index extra salt that allows to deploy more wallets if needed for same EOA (default 0)
     */
-    function getAddressForCounterfactualWallet(address _owner, uint _index) external view returns (address _wallet) {
+    function getAddressForCounterfactualWallet(address _owner, uint _index) public view returns (address _wallet) {
        bytes memory code = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
        bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
        bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, keccak256(code)));
diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccountNoAuth.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccountNoAuth.sol
index 8723472..435fb88 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccountNoAuth.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccountNoAuth.sol
@@ -163,15 +163,13 @@ contract SmartAccountNoAuth is
     // Initialize / Setup
     // Used to setup
     // i. owner ii. entry point address iii. handler
-    function init(address _owner, address _entryPointAddress, address _handler) public override initializer { 
+    function init(address _owner, address _entryPointAddress) public override initializer { 
         require(owner == address(0), "Already initialized");
         require(address(_entryPoint) == address(0), "Already initialized");
         require(_owner != address(0),"Invalid owner");
         require(_entryPointAddress != address(0), "Invalid Entrypoint");
-        require(_handler != address(0), "Invalid Entrypoint");
         owner = _owner;
         _entryPoint =  IEntryPoint(payable(_entryPointAddress));
-        if (_handler != address(0)) internalSetFallbackHandler(_handler);
         setupModules(address(0), bytes(""));
     }
 
diff --git a/scw-contracts/contracts/smart-contract-wallet/utils/GasEstimatorSmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/utils/GasEstimatorSmartAccount.sol
index b6cbf70..548542b 100644
--- a/scw-contracts/contracts/smart-contract-wallet/utils/GasEstimatorSmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/utils/GasEstimatorSmartAccount.sol
@@ -10,14 +10,12 @@ contract GasEstimatorSmartAccount {
     address _actualWallet,
     address _factory,
     address _owner,
-    address _entryPoint,
-    address _handler,
     uint _index,
     bytes calldata _data // execTransaction data // counterFactual wallet should have assets if required
   ) external returns (bool success, bytes memory result, uint256 gas) {
     // solhint-disable
     uint256 initialGas = gasleft();
-    address _wallet = SmartAccountFactory(_factory).deployCounterFactualWallet(_owner, _entryPoint, _handler, _index);
+    address _wallet = SmartAccountFactory(_factory).deployCounterFactualWallet(_owner, _index);
     (success, result) = _actualWallet.call(_data);
     gas = initialGas - gasleft();
     // solhint-enable