fix: factory getAddress should revert for invalid inputs to MultiOwne…

…rPlugin
alchemyplatform · Dec 12, 2023 · 30fdf9f · 30fdf9f
1 parent 0e3fd1e
commit 30fdf9f
Show file tree

Hide file tree

Showing 3 changed files with 86 additions and 12 deletions.
diff --git a/src/factory/MultiOwnerMSCAFactory.sol b/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,21 +45,20 @@ 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. 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) {
         bytes[] memory pluginInitBytes = new bytes[](1);
         pluginInitBytes[0] = abi.encode(owners);
 
-        bytes32 combinedSalt = _getSalt(salt, pluginInitBytes[0]);
-        addr = Create2.computeAddress(
-            combinedSalt, keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(IMPL, "")))
-        );
+        // getAddress checks and reverts if owners array is invalid in MultiOwnerPlugin
+        addr = getAddress(salt, owners);
 
         // 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 : _getSalt(salt, pluginInitBytes[0])}(IMPL, "");
 
             address[] memory plugins = new address[](1);
             plugins[0] = MULTI_OWNER_PLUGIN;
@@ -102,9 +105,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, "")))

diff --git a/src/factory/MultiOwnerTokenReceiverMSCAFactory.sol b/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,21 +51,20 @@ 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) {
         bytes[] memory pluginInitBytes = new bytes[](2); // empty bytes for TokenReceiverPlugin init
         pluginInitBytes[0] = abi.encode(owners);
 
-        bytes32 combinedSalt = _getSalt(salt, pluginInitBytes[0]);
-        addr = Create2.computeAddress(
-            combinedSalt, keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(IMPL, "")))
-        );
+        // getAddress checks and reverts if owners array is invalid in MultiOwnerPlugin
+        addr = getAddress(salt, owners);
 
         // 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 : _getSalt(salt, pluginInitBytes[0])}(IMPL, "");
 
             address[] memory plugins = new address[](2);
             plugins[0] = MULTI_OWNER_PLUGIN;
@@ -110,9 +113,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, "")))

diff --git a/test/factory/MultiOwnerMSCAFactoryTest.t.sol b/test/factory/MultiOwnerMSCAFactoryTest.t.sol
@@ -87,6 +87,22 @@ contract MultiOwnerMSCAFactoryTest is Test {
         assertEq(plugins[0], address(multiOwnerPlugin));
     }
 
+    function test_badOwnersArray() public {
+        vm.expectRevert(MultiOwnerMSCAFactory.OwnersArrayEmpty.selector);
+        factory.createAccount(0, new address[](0));
+
+        address[] memory badOwners = new address[](2);
+
+        vm.expectRevert(MultiOwnerMSCAFactory.ZeroAddressOwner.selector);
+        factory.createAccount(0, badOwners);
+
+        badOwners[0] = address(1);
+        badOwners[1] = address(1);
+
+        vm.expectRevert(MultiOwnerMSCAFactory.DuplicateOwner.selector);
+        factory.createAccount(0, badOwners);
+    }
+
     function test_addStake() public {
         assertEq(entryPoint.balanceOf(address(factory)), 0);
         vm.deal(address(this), 100 ether);