Merge pull request #114 from bcnmy/fix/security-h02

🔒 H-02 - Prevent Freezing of Funds in Factory Contracts

contracts/factory/BiconomyMetaFactory.sol

@@ -69,7 +69,7 @@ contract BiconomyMetaFactory is Stakeable {
     /// @return createdAccount The address of the newly created Nexus account.
     function deployWithFactory(address factory, bytes calldata factoryData) external payable returns (address payable createdAccount) {
         require(factoryWhitelist[address(factory)], FactoryNotWhitelisted());
-        (bool success, bytes memory returnData) = factory.call{value: msg.value}(factoryData);
+        (bool success, bytes memory returnData) = factory.call{ value: msg.value }(factoryData);
 
         // Check if the call was successful
         require(success, CallToDeployWithFactoryFailed());

contracts/factory/K1ValidatorFactory.sol

@@ -47,6 +47,9 @@ contract K1ValidatorFactory is Stakeable {
     /// @notice Error thrown when a zero address is provided for the implementation, K1 validator, or bootstrapper.
     error ZeroAddressNotAllowed();
 
+    /// @notice Error thrown when an inner call fails.
+    error InnerCallFailed();
+
     /// @notice Constructor to set the immutable variables.
     /// @param implementation The address of the Nexus implementation to be used for all deployments.
     /// @param factoryOwner The address of the factory owner.
@@ -100,6 +103,9 @@ contract K1ValidatorFactory is Stakeable {
         if (!alreadyDeployed) {
             INexus(account).initializeAccount(initData);
             emit AccountCreated(account, eoaOwner, index);
+        } else if (msg.value > 0) {
+            (bool success, ) = eoaOwner.call{ value: msg.value }("");
+            require(success, InnerCallFailed());
         }
         return payable(account);
     }

contracts/factory/NexusAccountFactory.sol

@@ -58,6 +58,8 @@ contract NexusAccountFactory is Stakeable, INexusFactory {
         if (!alreadyDeployed) {
             INexus(account).initializeAccount(initData);
             emit AccountCreated(account, initData, salt);
+        } else if (msg.value > 0) {
+            revert AccountAlreadyDeployed(account);
         }
         return payable(account);
     }

contracts/factory/RegistryFactory.sol

@@ -125,6 +125,8 @@ contract RegistryFactory is Stakeable, INexusFactory {
         if (!alreadyDeployed) {
             INexus(account).initializeAccount(initData);
             emit AccountCreated(account, initData, salt);
+        } else if (msg.value > 0) {
+            revert AccountAlreadyDeployed(account);
         }
         return payable(account);
     }

contracts/interfaces/factory/INexusFactory.sol

@@ -21,8 +21,15 @@ pragma solidity ^0.8.26;
 /// Special thanks to the Solady team for foundational contributions: https://github.com/Vectorized/solady
 interface INexusFactory {
     /// @notice Emitted when a new Smart Account is created.
+    /// @param account The address of the newly created account.
+    /// @param initData Initialization data used for the new Smart Account.
+    /// @param salt Unique salt used during the creation of the Smart Account.
     event AccountCreated(address indexed account, bytes indexed initData, bytes32 indexed salt);
 
+    /// @notice Error indicating that the account is already deployed
+    /// @param account The address of the account that is already deployed
+    error AccountAlreadyDeployed(address account);
+
     /// @notice Error thrown when the owner address is zero.
     error ZeroAddressNotAllowed();
 

contracts/utils/RegistryBootstrap.sol

@@ -42,8 +42,8 @@ contract Bootstrap is ModuleManager {
         address[] calldata attesters,
         uint8 threshold
     ) external {
-        _installValidator(address(validator), data);
         _configureRegistry(registry, attesters, threshold);
+        _installValidator(address(validator), data);
     }
 
     /// @notice Initializes the Nexus account with multiple modules.
@@ -61,6 +61,8 @@ contract Bootstrap is ModuleManager {
         address[] calldata attesters,
         uint8 threshold
     ) external {
+        _configureRegistry(registry, attesters, threshold);
+
         // Initialize validators
         for (uint256 i = 0; i < validators.length; i++) {
             _installValidator(validators[i].module, validators[i].data);
@@ -82,8 +84,6 @@ contract Bootstrap is ModuleManager {
             if (fallbacks[i].module == address(0)) continue;
             _installFallbackHandler(fallbacks[i].module, fallbacks[i].data);
         }
-
-        _configureRegistry(registry, attesters, threshold);
     }
 
     /// @notice Initializes the Nexus account with a scoped set of modules.
@@ -97,6 +97,8 @@ contract Bootstrap is ModuleManager {
         address[] calldata attesters,
         uint8 threshold
     ) external {
+        _configureRegistry(registry, attesters, threshold);
+
         // Initialize validators
         for (uint256 i = 0; i < validators.length; i++) {
             _installValidator(validators[i].module, validators[i].data);
@@ -106,8 +108,6 @@ contract Bootstrap is ModuleManager {
         if (hook.module != address(0)) {
             _installHook(hook.module, hook.data);
         }
-
-        _configureRegistry(registry, attesters, threshold);
     }
 
     /// @notice Prepares calldata for the initNexus function.

test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.t.sol

@@ -110,16 +110,18 @@ contract TestK1ValidatorFactory_Deployments is NexusTest_Base {
         assertEq(deployedAccountAddress, expectedAddress, "Computed address mismatch");
     }
 
-    /// @notice Tests that creating an account with the same owner and index results in the same address.
-    function test_CreateAccount_SameOwnerAndIndex() public payable {
-        uint256 index = 0;
-        address expectedOwner = user.addr;
+/// @notice Tests that creating an account with the same owner and index results in the same address.
+function test_CreateAccount_SameOwnerAndIndex() public payable {
+    uint256 index = 0;
+    address expectedOwner = user.addr;
 
-        address payable firstAccountAddress = validatorFactory.createAccount{ value: 1 ether }(expectedOwner, index, ATTESTERS, THRESHOLD);
-        address payable secondAccountAddress = validatorFactory.createAccount{ value: 1 ether }(expectedOwner, index, ATTESTERS, THRESHOLD);
+    // Create the first account with the given owner and index
+    address payable firstAccountAddress = validatorFactory.createAccount{ value: 1 ether }(expectedOwner, index, ATTESTERS, THRESHOLD);
 
-        assertEq(firstAccountAddress, secondAccountAddress, "Addresses should match for the same owner and index");
-    }
+    // Expect the second call to revert with InnerCallFailed
+    vm.expectRevert(K1ValidatorFactory.InnerCallFailed.selector);
+    address payable secondAccountAddress = validatorFactory.createAccount{ value: 1 ether }(expectedOwner, index, ATTESTERS, THRESHOLD);
+}
 
     /// @notice Tests that creating accounts with different indexes results in different addresses.
     function test_CreateAccount_DifferentIndexes() public payable {