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] 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);