fix: [spearbit-97] Prevent createAccount() user error (#16)

src/factory/MultiOwnerMSCAFactory.sol

@@ -21,6 +21,10 @@ contract MultiOwnerMSCAFactory is Ownable {
     bytes32 internal immutable _MULTI_OWNER_PLUGIN_MANIFEST_HASH;
     IEntryPoint public immutable ENTRYPOINT;
 
+    error OwnersArrayEmpty();
+    error ZeroAddressOwner();
+    error DuplicateOwner();
+
     /// @notice Constructor for the factory
     constructor(
         address owner,
@@ -41,6 +45,7 @@ contract MultiOwnerMSCAFactory is Ownable {
 
     /// @notice Create a modular smart contract account
     /// @dev Account address depends on salt, impl addr, plugins and plugin init data
+    /// @dev Owner array must be valid else the plugin installation step would revert. See getAddress below
     /// @param salt salt for additional entropy for create2
     /// @param owners address array of the owners
     function createAccount(uint256 salt, address[] calldata owners) external returns (address addr) {
@@ -55,7 +60,7 @@ contract MultiOwnerMSCAFactory is Ownable {
         // short circuit if exists
         if (addr.code.length == 0) {
             // not necessary to check return addr of this arg since next call fails if so
-            new ERC1967Proxy{salt : combinedSalt}(IMPL, "");
+            new ERC1967Proxy{salt: combinedSalt}(IMPL, "");
 
             address[] memory plugins = new address[](1);
             plugins[0] = MULTI_OWNER_PLUGIN;
@@ -102,9 +107,35 @@ contract MultiOwnerMSCAFactory is Ownable {
     }
 
     /// @notice Getter for counterfactual address based on input params
+    /// @dev Owner array cannot be empty, cannot contain address(0), and cannot contain duplicates
     /// @param salt salt for additional entropy for create2
     /// @param owners array of addresses of the owner
-    function getAddress(uint256 salt, address[] calldata owners) external view returns (address) {
+    function getAddress(uint256 salt, address[] calldata owners) public view returns (address) {
+        // array can't be empty
+        if (owners.length == 0) {
+            revert OwnersArrayEmpty();
+        }
+
+        for (uint256 i = 0; i < owners.length;) {
+            // array can't contain address(0)
+            if (owners[i] == address(0)) {
+                revert ZeroAddressOwner();
+            }
+
+            for (uint256 j = i + 1; j < owners.length;) {
+                // array can't have matching elements
+                if (owners[i] == owners[j]) {
+                    revert DuplicateOwner();
+                }
+                unchecked {
+                    ++j;
+                }
+            }
+            unchecked {
+                ++i;
+            }
+        }
+
         return Create2.computeAddress(
             _getSalt(salt, abi.encode(owners)),
             keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(IMPL, "")))

src/factory/MultiOwnerTokenReceiverMSCAFactory.sol

@@ -23,6 +23,10 @@ contract MultiOwnerTokenReceiverMSCAFactory is Ownable {
     bytes32 internal immutable _TOKEN_RECEIVER_PLUGIN_MANIFEST_HASH;
     IEntryPoint public immutable ENTRYPOINT;
 
+    error OwnersArrayEmpty();
+    error ZeroAddressOwner();
+    error DuplicateOwner();
+
     /// @notice Constructor for the factory
     constructor(
         address owner,
@@ -47,6 +51,7 @@ contract MultiOwnerTokenReceiverMSCAFactory is Ownable {
 
     /// @notice Create a modular smart contract account
     /// @dev Account address depends on salt, impl addr, plugins and plugin init data
+    /// @dev Owner array must be valid. See getAddress below
     /// @param salt salt for additional entropy for create2
     /// @param owners address array of the owners
     function createAccount(uint256 salt, address[] calldata owners) external returns (address addr) {
@@ -61,7 +66,7 @@ contract MultiOwnerTokenReceiverMSCAFactory is Ownable {
         // short circuit if exists
         if (addr.code.length == 0) {
             // not necessary to check return addr of this arg since next call fails if so
-            new ERC1967Proxy{salt : combinedSalt}(IMPL, "");
+            new ERC1967Proxy{salt: combinedSalt}(IMPL, "");
 
             address[] memory plugins = new address[](2);
             plugins[0] = MULTI_OWNER_PLUGIN;
@@ -110,9 +115,35 @@ contract MultiOwnerTokenReceiverMSCAFactory is Ownable {
     }
 
     /// @notice Getter for counterfactual address based on input params
+    /// @dev Owner array cannot be empty, cannot contain address(0), and cannot contain duplicates
     /// @param salt salt for additional entropy for create2
     /// @param owners array of addresses of the owner
-    function getAddress(uint256 salt, address[] calldata owners) external view returns (address) {
+    function getAddress(uint256 salt, address[] calldata owners) public view returns (address) {
+        // array can't be empty
+        if (owners.length == 0) {
+            revert OwnersArrayEmpty();
+        }
+
+        for (uint256 i = 0; i < owners.length;) {
+            // array can't contain address(0)
+            if (owners[i] == address(0)) {
+                revert ZeroAddressOwner();
+            }
+
+            for (uint256 j = i + 1; j < owners.length;) {
+                // array can't have matching elements
+                if (owners[i] == owners[j]) {
+                    revert DuplicateOwner();
+                }
+                unchecked {
+                    ++j;
+                }
+            }
+            unchecked {
+                ++i;
+            }
+        }
+
         return Create2.computeAddress(
             _getSalt(salt, abi.encode(owners)),
             keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(IMPL, "")))

test/factory/MultiOwnerMSCAFactoryTest.t.sol

@@ -37,11 +37,7 @@ contract MultiOwnerMSCAFactoryTest is Test {
         multiOwnerPlugin = new MultiOwnerPlugin();
         bytes32 manifestHash = keccak256(abi.encode(multiOwnerPlugin.pluginManifest()));
         factory = new MultiOwnerMSCAFactory(
-            address(this), 
-            address(multiOwnerPlugin), 
-            impl, 
-            manifestHash, 
-            IEntryPoint(address(entryPoint))
+            address(this), address(multiOwnerPlugin), impl, manifestHash, IEntryPoint(address(entryPoint))
         );
         vm.deal(address(this), 100 ether);
     }
@@ -87,6 +83,22 @@ contract MultiOwnerMSCAFactoryTest is Test {
         assertEq(plugins[0], address(multiOwnerPlugin));
     }
 
+    function test_badOwnersArray() public {
+        vm.expectRevert(MultiOwnerMSCAFactory.OwnersArrayEmpty.selector);
+        factory.getAddress(0, new address[](0));
+
+        address[] memory badOwners = new address[](2);
+
+        vm.expectRevert(MultiOwnerMSCAFactory.ZeroAddressOwner.selector);
+        factory.getAddress(0, badOwners);
+
+        badOwners[0] = address(1);
+        badOwners[1] = address(1);
+
+        vm.expectRevert(MultiOwnerMSCAFactory.DuplicateOwner.selector);
+        factory.getAddress(0, badOwners);
+    }
+
     function test_addStake() public {
         assertEq(entryPoint.balanceOf(address(factory)), 0);
         vm.deal(address(this), 100 ether);

test/factory/MultiOwnerTokenReceiverFactoryTest.t.sol

@@ -54,11 +54,11 @@ contract MultiOwnerTokenReceiverMSCAFactoryTest is Test {
         bytes32 ownerManifestHash = keccak256(abi.encode(multiOwnerPlugin.pluginManifest()));
         bytes32 tokenReceiverManifestHash = keccak256(abi.encode(tokenReceiverPlugin.pluginManifest()));
         factory = new MultiOwnerTokenReceiverMSCAFactory(
-            address(this), 
-            address(multiOwnerPlugin), 
+            address(this),
+            address(multiOwnerPlugin),
             address(tokenReceiverPlugin),
-            impl, 
-            ownerManifestHash, 
+            impl,
+            ownerManifestHash,
             tokenReceiverManifestHash,
             IEntryPoint(address(entryPoint))
         );