Fix/finding #2 Fix Calculating Salt

contracts/factory/K1ValidatorFactory.sol

@@ -72,6 +72,8 @@ contract K1ValidatorFactory is Stakeable {
     /// @notice Creates a new Nexus with a specific validator and initialization data.
     /// @param eoaOwner The address of the EOA owner of the Nexus.
     /// @param index The index of the Nexus.
+    /// @param attesters The list of attesters for the Nexus.
+    /// @param threshold The threshold for the Nexus.
     /// @return The address of the newly created Nexus.
     function createAccount(
         address eoaOwner,
@@ -80,14 +82,7 @@ contract K1ValidatorFactory is Stakeable {
         uint8 threshold
     ) external payable returns (address payable) {
         // Compute the actual salt for deterministic deployment
-        bytes32 actualSalt;
-        assembly {
-            let ptr := mload(0x40)
-            let calldataLength := sub(calldatasize(), 0x04)
-            mstore(0x40, add(ptr, calldataLength))
-            calldatacopy(ptr, 0x04, calldataLength)
-            actualSalt := keccak256(ptr, calldataLength)
-        }
+        bytes32 actualSalt = keccak256(abi.encodePacked(eoaOwner, index, attesters, threshold));
 
         // Deploy the Nexus contract using the computed salt
         (bool alreadyDeployed, address account) = LibClone.createDeterministicERC1967(msg.value, ACCOUNT_IMPLEMENTATION, actualSalt);
@@ -105,19 +100,19 @@ contract K1ValidatorFactory is Stakeable {
     }
 
     /// @notice Computes the expected address of a Nexus contract using the factory's deterministic deployment algorithm.
-    /// @param - The address of the EOA owner of the Nexus.
-    /// @param - The index of the Nexus.
+    /// @param eoaOwner The address of the EOA owner of the Nexus.
+    /// @param index The index of the Nexus.
+    /// @param attesters The list of attesters for the Nexus.
+    /// @param threshold The threshold for the Nexus.
     /// @return expectedAddress The expected address at which the Nexus contract will be deployed if the provided parameters are used.
-    function computeAccountAddress(address, uint256, address[] calldata, uint8) external view returns (address payable expectedAddress) {
+    function computeAccountAddress(
+        address eoaOwner,
+        uint256 index,
+        address[] calldata attesters,
+        uint8 threshold
+    ) external view returns (address payable expectedAddress) {
         // Compute the actual salt for deterministic deployment
-        bytes32 actualSalt;
-        assembly {
-            let ptr := mload(0x40)
-            let calldataLength := sub(calldatasize(), 0x04)
-            mstore(0x40, add(ptr, calldataLength))
-            calldatacopy(ptr, 0x04, calldataLength)
-            actualSalt := keccak256(ptr, calldataLength)
-        }
+        bytes32 actualSalt = keccak256(abi.encodePacked(eoaOwner, index, attesters, threshold));
 
         // Predict the deterministic address using the LibClone library
         expectedAddress = payable(LibClone.predictDeterministicAddressERC1967(ACCOUNT_IMPLEMENTATION, actualSalt, address(this)));

contracts/factory/NexusAccountFactory.sol

@@ -42,14 +42,17 @@
     /// @param salt Unique salt for the Smart Account creation.
     /// @return The address of the newly created Nexus account.
     function createAccount(bytes calldata initData, bytes32 salt) external payable override returns (address payable) {
-        // Compute the actual salt for deterministic deployment
+        // Compute the actual salt as the hash of initData and salt using assembly
         bytes32 actualSalt;
         assembly {
+            // Load the free memory pointer
             let ptr := mload(0x40)
-            let calldataLength := sub(calldatasize(), 0x04)
-            mstore(0x40, add(ptr, calldataLength))
-            calldatacopy(ptr, 0x04, calldataLength)
-            actualSalt := keccak256(ptr, calldataLength)
+            // Store initData at the free memory pointer
+            calldatacopy(ptr, initData.offset, initData.length)
+            // Store salt after initData
+            mstore(add(ptr, initData.length), salt)
+            // Compute the keccak256 hash of the concatenated initData and salt
+            actualSalt := keccak256(ptr, add(initData.length, 32))
         }
 
         // Deploy the account using the deterministic address
@@ -63,20 +66,22 @@
     }
 
     /// @notice Computes the expected address of a Nexus contract using the factory's deterministic deployment algorithm.
-    /// @param - Initialization data to be called on the new Smart Account.
-    /// @param - Unique salt for the Smart Account creation.
+    /// @param initData - Initialization data to be called on the new Smart Account.
+    /// @param salt - Unique salt for the Smart Account creation.
     /// @return expectedAddress The expected address at which the Nexus contract will be deployed if the provided parameters are used.
-    function computeAccountAddress(bytes calldata, bytes32) external view override returns (address payable expectedAddress) {
+    function computeAccountAddress(bytes calldata initData, bytes32 salt) external view override returns (address payable expectedAddress) {
         // Compute the actual salt for deterministic deployment
         bytes32 actualSalt;
         assembly {
+            // Load the free memory pointer
             let ptr := mload(0x40)
-            let calldataLength := sub(calldatasize(), 0x04)
-            mstore(0x40, add(ptr, calldataLength))
-            calldatacopy(ptr, 0x04, calldataLength)
-            actualSalt := keccak256(ptr, calldataLength)
+            // Store initData at the free memory pointer
+            calldatacopy(ptr, initData.offset, initData.length)
+            // Store salt after initData
+            mstore(add(ptr, initData.length), salt)
+            // Compute the keccak256 hash of the concatenated initData and salt
+            actualSalt := keccak256(ptr, add(initData.length, 32))
         }
-
         expectedAddress = payable(LibClone.predictDeterministicAddressERC1967(ACCOUNT_IMPLEMENTATION, actualSalt, address(this)));
     }
 }

contracts/factory/RegistryFactory.sol

@@ -96,27 +96,30 @@
 
         // Ensure all modules are whitelisted
         for (uint256 i = 0; i < validators.length; i++) {
-            require(isModuleAllowed(validators[i].module, MODULE_TYPE_VALIDATOR), ModuleNotWhitelisted(validators[i].module));
+            require(_isModuleAllowed(validators[i].module, MODULE_TYPE_VALIDATOR), ModuleNotWhitelisted(validators[i].module));
         }
 
         for (uint256 i = 0; i < executors.length; i++) {
-            require(isModuleAllowed(executors[i].module, MODULE_TYPE_EXECUTOR), ModuleNotWhitelisted(executors[i].module));
+            require(_isModuleAllowed(executors[i].module, MODULE_TYPE_EXECUTOR), ModuleNotWhitelisted(executors[i].module));
         }
 
-        require(isModuleAllowed(hook.module, MODULE_TYPE_HOOK), ModuleNotWhitelisted(hook.module));
+        require(_isModuleAllowed(hook.module, MODULE_TYPE_HOOK), ModuleNotWhitelisted(hook.module));
 
         for (uint256 i = 0; i < fallbacks.length; i++) {
-            require(isModuleAllowed(fallbacks[i].module, MODULE_TYPE_FALLBACK), ModuleNotWhitelisted(fallbacks[i].module));
+            require(_isModuleAllowed(fallbacks[i].module, MODULE_TYPE_FALLBACK), ModuleNotWhitelisted(fallbacks[i].module));
         }
 
-        // Compute the actual salt for deterministic deployment
+        // Compute the actual salt as the hash of initData and salt using assembly
         bytes32 actualSalt;
         assembly {
+            // Load the free memory pointer
             let ptr := mload(0x40)
-            let calldataLength := sub(calldatasize(), 0x04)
-            mstore(0x40, add(ptr, calldataLength))
-            calldatacopy(ptr, 0x04, calldataLength)
-            actualSalt := keccak256(ptr, calldataLength)
+            // Store initData at the free memory pointer
+            calldatacopy(ptr, initData.offset, initData.length)
+            // Store salt after initData
+            mstore(add(ptr, initData.length), salt)
+            // Compute the keccak256 hash of the concatenated initData and salt
+            actualSalt := keccak256(ptr, add(initData.length, 32))
         }
 
         // Deploy the account using the deterministic address
@@ -130,25 +133,30 @@
     }
 
     /// @notice Computes the expected address of a Nexus contract using the factory's deterministic deployment algorithm.
-    /// @param - Initialization data to be called on the new Smart Account.
-    /// @param - Unique salt for the Smart Account creation.
+    /// @param initData - Initialization data to be called on the new Smart Account.
+    /// @param salt - Unique salt for the Smart Account creation.
     /// @return expectedAddress The expected address at which the Nexus contract will be deployed if the provided parameters are used.
-    function computeAccountAddress(bytes calldata, bytes32) external view override returns (address payable expectedAddress) {
+    function computeAccountAddress(bytes calldata initData, bytes32 salt) external view override returns (address payable expectedAddress) {
+        // Compute the actual salt as the hash of initData and salt using assembly
         bytes32 actualSalt;
         assembly {
+            // Load the free memory pointer
             let ptr := mload(0x40)
-            let calldataLength := sub(calldatasize(), 0x04)
-            mstore(0x40, add(ptr, calldataLength))
-            calldatacopy(ptr, 0x04, calldataLength)
-            actualSalt := keccak256(ptr, calldataLength)
+            // Store initData at the free memory pointer
+            calldatacopy(ptr, initData.offset, initData.length)
+            // Store salt after initData
+            mstore(add(ptr, initData.length), salt)
+            // Compute the keccak256 hash of the concatenated initData and salt
+            actualSalt := keccak256(ptr, add(initData.length, 32))
         }
         expectedAddress = payable(LibClone.predictDeterministicAddressERC1967(ACCOUNT_IMPLEMENTATION, actualSalt, address(this)));
     }
 
     /// @notice Checks if a module is whitelisted.
     /// @param module The address of the module to check.
-    /// @return True if the module is whitelisted, false otherwise.
-    function isModuleAllowed(address module, uint256 moduleType) public view returns (bool) {
+    /// @param moduleType The type of the module to check.
+    /// @return True if the module is whitelisted, reverts otherwise.
+    function _isModuleAllowed(address module, uint256 moduleType) private view returns (bool) {
         REGISTRY.check(module, moduleType, attesters, threshold);
         return true;
     }

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

@@ -205,14 +205,40 @@ contract TestAccountFactory_Deployments is NexusTest_Base {
 
         // Verify that the validators and hook were installed
         assertTrue(
-            INexus(deployedAccountAddress).isModuleInstalled(MODULE_TYPE_VALIDATOR, address(VALIDATOR_MODULE), ""), "Validator should be installed"
+            INexus(deployedAccountAddress).isModuleInstalled(MODULE_TYPE_VALIDATOR, address(VALIDATOR_MODULE), ""),
+            "Validator should be installed"
         );
         assertTrue(
             INexus(deployedAccountAddress).isModuleInstalled(MODULE_TYPE_HOOK, address(HOOK_MODULE), abi.encodePacked(user.addr)),
             "Hook should be installed"
         );
     }
 
+    /// @notice Tests that the manually computed address matches the one from computeAccountAddress.
+    function test_ComputeAccountAddress_ManualComparison() public {
+        // Prepare the initial data and salt
+        BootstrapConfig[] memory validators = BootstrapLib.createArrayConfig(address(VALIDATOR_MODULE), initData);
+        BootstrapConfig memory hook = BootstrapLib.createSingleConfig(address(0), "");
+        bytes memory saDeploymentIndex = "0";
+        bytes32 salt = keccak256(saDeploymentIndex);
+
+        // Create initcode and salt to be sent to Factory
+        bytes memory _initData = BOOTSTRAPPER.getInitNexusScopedCalldata(validators, hook, REGISTRY, ATTESTERS, THRESHOLD);
+
+        // Manually compute the actual salt
+        bytes32 actualSalt = keccak256(abi.encodePacked(_initData, salt));
+        // Compute the expected address using the factory's function
+        address payable expectedAddress = FACTORY.computeAccountAddress(_initData, salt);
+
+        // Manually compute the expected address
+        address payable manualExpectedAddress = payable(
+            LibClone.predictDeterministicAddressERC1967(FACTORY.ACCOUNT_IMPLEMENTATION(), actualSalt, address(FACTORY))
+        );
+
+        // Validate that both addresses match
+        assertEq(expectedAddress, manualExpectedAddress, "Manually computed address mismatch");
+    }
+
     /// @notice Tests that the Nexus contract constructor reverts if the entry point address is zero.
     function test_Constructor_RevertIf_EntryPointIsZero() public {
         address zeroAddress = address(0);

test/foundry/unit/concrete/factory/TestAccountFactory_Deployments.tree

@@ -20,5 +20,7 @@ TestAccountFactory_Deployments
     │   └── it should deploy the account correctly
     ├── when initializing Nexus with a hook module and deploying an account
     │   └── it should deploy the account and install the modules correctly
-    └── when the Nexus contract constructor is called with a zero entry point address
-        └── it should revert
+    ├── when the Nexus contract constructor is called with a zero entry point address
+    │   └── it should revert
+    └── when manually computing the address using keccak256
+        └── it should match the address computed by computeAccountAddress

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

@@ -21,8 +21,13 @@ contract TestK1ValidatorFactory_Deployments is NexusTest_Base {
         vm.deal(user.addr, 1 ether);
         initData = abi.encodePacked(user.addr);
         bootstrapper = new Bootstrap();
-        validatorFactory =
-            new K1ValidatorFactory(address(ACCOUNT_IMPLEMENTATION), address(FACTORY_OWNER.addr), address(VALIDATOR_MODULE), bootstrapper, REGISTRY);
+        validatorFactory = new K1ValidatorFactory(
+            address(ACCOUNT_IMPLEMENTATION),
+            address(FACTORY_OWNER.addr),
+            address(VALIDATOR_MODULE),
+            bootstrapper,
+            REGISTRY
+        );
     }
 
     /// @notice Tests if the constructor correctly initializes the factory with the given implementation, K1 Validator, and Bootstrapper addresses.
@@ -132,4 +137,26 @@ contract TestK1ValidatorFactory_Deployments is NexusTest_Base {
 
         assertTrue(accountAddress0 != accountAddress1, "Accounts with different indexes should have different addresses");
     }
+
+    /// @notice Tests that the computed address matches the manually computed address using keccak256.
+    function test_ComputeAccountAddress_MatchesManualComputation() public {
+        address eoaOwner = user.addr;
+        uint256 index = 1;
+        address[] memory attesters = new address[](1);
+        attesters[0] = address(0x1234);
+        uint8 threshold = 1;
+
+        // Compute the actual salt manually using keccak256
+        bytes32 manualSalt = keccak256(abi.encodePacked(eoaOwner, index, attesters, threshold));
+
+        address expectedAddress = LibClone.predictDeterministicAddressERC1967(
+            address(validatorFactory.ACCOUNT_IMPLEMENTATION()),
+            manualSalt,
+            address(validatorFactory)
+        );
+
+        address computedAddress = validatorFactory.computeAccountAddress(eoaOwner, index, attesters, threshold);
+
+        assertEq(expectedAddress, computedAddress, "Computed address does not match manually computed address");
+    }
 }

test/foundry/unit/concrete/factory/TestK1ValidatorFactory_Deployments.tree

@@ -11,5 +11,7 @@ TestK1ValidatorFactory_Deployments
     │   └── it should return the expected address
     ├── when creating an account with the same owner and index
     │   └── it should result in the same address
-    └── when creating accounts with different indexes
-        └── it should result in different addresses
+    ├── when creating accounts with different indexes
+    │   └── it should result in different addresses
+    └── when manually computing the address using keccak256
+        └── it should match the address computed by computeAccountAddress

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

@@ -227,7 +227,8 @@ contract TestNexusAccountFactory_Deployments is NexusTest_Base {
 
         // Verify that the validators and hook were installed
         assertTrue(
-            INexus(deployedAccountAddress).isModuleInstalled(MODULE_TYPE_VALIDATOR, address(VALIDATOR_MODULE), ""), "Validator should be installed"
+            INexus(deployedAccountAddress).isModuleInstalled(MODULE_TYPE_VALIDATOR, address(VALIDATOR_MODULE), ""),
+            "Validator should be installed"
         );
         assertTrue(
             INexus(deployedAccountAddress).isModuleInstalled(MODULE_TYPE_HOOK, address(HOOK_MODULE), abi.encodePacked(user.addr)),