From 30fdf9f22f5d6b1466ade055931e11ab25b58637 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Tue, 12 Dec 2023 15:00:11 -0500 Subject: [PATCH 1/2] fix: factory getAddress should revert for invalid inputs to MultiOwnerPlugin --- src/factory/MultiOwnerMSCAFactory.sol | 41 ++++++++++++++++--- .../MultiOwnerTokenReceiverMSCAFactory.sol | 41 ++++++++++++++++--- test/factory/MultiOwnerMSCAFactoryTest.t.sol | 16 ++++++++ 3 files changed, 86 insertions(+), 12 deletions(-) diff --git a/src/factory/MultiOwnerMSCAFactory.sol b/src/factory/MultiOwnerMSCAFactory.sol index e859ce92..1c247411 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,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 index 53a25927..b5504df6 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,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 index c5477ece..66cd55d0 100644 --- 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); From b4a04104e85797820e14567f9d6754b1b4b6a6d8 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Wed, 13 Dec 2023 19:51:14 -0500 Subject: [PATCH 2/2] fix: address pr review comments --- src/factory/MultiOwnerMSCAFactory.sol | 10 ++++++---- src/factory/MultiOwnerTokenReceiverMSCAFactory.sol | 8 +++++--- test/factory/MultiOwnerMSCAFactoryTest.t.sol | 12 ++++-------- .../factory/MultiOwnerTokenReceiverFactoryTest.t.sol | 8 ++++---- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/factory/MultiOwnerMSCAFactory.sol b/src/factory/MultiOwnerMSCAFactory.sol index 1c247411..26184cc2 100644 --- a/src/factory/MultiOwnerMSCAFactory.sol +++ b/src/factory/MultiOwnerMSCAFactory.sol @@ -45,20 +45,22 @@ 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 + /// @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) { bytes[] memory pluginInitBytes = new bytes[](1); pluginInitBytes[0] = abi.encode(owners); - // getAddress checks and reverts if owners array is invalid in MultiOwnerPlugin - addr = getAddress(salt, owners); + bytes32 combinedSalt = _getSalt(salt, pluginInitBytes[0]); + addr = Create2.computeAddress( + combinedSalt, keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(IMPL, ""))) + ); // 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 : _getSalt(salt, pluginInitBytes[0])}(IMPL, ""); + new ERC1967Proxy{salt: combinedSalt}(IMPL, ""); address[] memory plugins = new address[](1); plugins[0] = MULTI_OWNER_PLUGIN; diff --git a/src/factory/MultiOwnerTokenReceiverMSCAFactory.sol b/src/factory/MultiOwnerTokenReceiverMSCAFactory.sol index b5504df6..0e0c8aa8 100644 --- a/src/factory/MultiOwnerTokenReceiverMSCAFactory.sol +++ b/src/factory/MultiOwnerTokenReceiverMSCAFactory.sol @@ -58,13 +58,15 @@ contract MultiOwnerTokenReceiverMSCAFactory is Ownable { bytes[] memory pluginInitBytes = new bytes[](2); // empty bytes for TokenReceiverPlugin init pluginInitBytes[0] = abi.encode(owners); - // getAddress checks and reverts if owners array is invalid in MultiOwnerPlugin - addr = getAddress(salt, owners); + bytes32 combinedSalt = _getSalt(salt, pluginInitBytes[0]); + addr = Create2.computeAddress( + combinedSalt, keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(IMPL, ""))) + ); // 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 : _getSalt(salt, pluginInitBytes[0])}(IMPL, ""); + new ERC1967Proxy{salt: combinedSalt}(IMPL, ""); address[] memory plugins = new address[](2); plugins[0] = MULTI_OWNER_PLUGIN; diff --git a/test/factory/MultiOwnerMSCAFactoryTest.t.sol b/test/factory/MultiOwnerMSCAFactoryTest.t.sol index 66cd55d0..6b4e2101 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); } @@ -89,18 +85,18 @@ contract MultiOwnerMSCAFactoryTest is Test { function test_badOwnersArray() public { vm.expectRevert(MultiOwnerMSCAFactory.OwnersArrayEmpty.selector); - factory.createAccount(0, new address[](0)); + factory.getAddress(0, new address[](0)); address[] memory badOwners = new address[](2); vm.expectRevert(MultiOwnerMSCAFactory.ZeroAddressOwner.selector); - factory.createAccount(0, badOwners); + factory.getAddress(0, badOwners); badOwners[0] = address(1); badOwners[1] = address(1); vm.expectRevert(MultiOwnerMSCAFactory.DuplicateOwner.selector); - factory.createAccount(0, badOwners); + factory.getAddress(0, badOwners); } function test_addStake() public { diff --git a/test/factory/MultiOwnerTokenReceiverFactoryTest.t.sol b/test/factory/MultiOwnerTokenReceiverFactoryTest.t.sol index d3dfbd4d..3d5116ff 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)) );