diff --git a/src/factory/MultiOwnerMSCAFactory.sol b/src/factory/MultiOwnerMSCAFactory.sol index e859ce920..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) { @@ -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, ""))) diff --git a/src/factory/MultiOwnerTokenReceiverMSCAFactory.sol b/src/factory/MultiOwnerTokenReceiverMSCAFactory.sol index 53a25927f..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) { @@ -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, ""))) diff --git a/test/factory/MultiOwnerMSCAFactoryTest.t.sol b/test/factory/MultiOwnerMSCAFactoryTest.t.sol index c5477ece1..6b4e2101b 100644 --- a/test/factory/MultiOwnerMSCAFactoryTest.t.sol +++ b/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); diff --git a/test/factory/MultiOwnerTokenReceiverFactoryTest.t.sol b/test/factory/MultiOwnerTokenReceiverFactoryTest.t.sol index d3dfbd4df..3d5116ff2 100644 --- a/test/factory/MultiOwnerTokenReceiverFactoryTest.t.sol +++ b/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)) );