diff --git a/src/factory/MultiOwnerMSCAFactory.sol b/src/factory/MultiOwnerMSCAFactory.sol index 1429124c9..26184cc24 100644 --- 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,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) { @@ -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, ""))) diff --git a/src/factory/MultiOwnerTokenReceiverMSCAFactory.sol b/src/factory/MultiOwnerTokenReceiverMSCAFactory.sol index 2461b6a81..0e0c8aa87 100644 --- 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,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) { @@ -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, ""))) diff --git a/test/factory/MultiOwnerMSCAFactoryTest.t.sol b/test/factory/MultiOwnerMSCAFactoryTest.t.sol index 62aff26a5..6b4e2101b 100644 --- a/test/factory/MultiOwnerMSCAFactoryTest.t.sol +++ b/test/factory/MultiOwnerMSCAFactoryTest.t.sol @@ -83,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);