diff --git a/.gas-snapshot b/.gas-snapshot index 2b94e55..5cff911 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -5,15 +5,15 @@ MultiOwnerPluginIntegration:test_userOpValidation_owner_standardExecute() (gas: MultiOwnerPluginTest:testFuzz_isValidSignature_ContractOwner(bytes32) (runs: 256, μ: 110067, ~: 110067) MultiOwnerPluginTest:testFuzz_isValidSignature_ContractOwnerWithEOAOwner(bytes32) (runs: 256, μ: 120381, ~: 120381) MultiOwnerPluginTest:testFuzz_isValidSignature_EOAOwner(string,bytes32) (runs: 256, μ: 130741, ~: 130734) -MultiOwnerPluginTest:testFuzz_isValidSignature_PasskeyOwner(bytes32) (runs: 256, μ: 365479, ~: 365612) +MultiOwnerPluginTest:testFuzz_isValidSignature_PasskeyOwner(bytes32) (runs: 256, μ: 365647, ~: 365955) MultiOwnerPluginTest:testFuzz_runtimeValidationFunction_BadFunctionId(uint8) (runs: 256, μ: 9747, ~: 9747) MultiOwnerPluginTest:testFuzz_userOpValidationFunction_BadFunctionId(uint8) (runs: 256, μ: 10744, ~: 10744) MultiOwnerPluginTest:testFuzz_userOpValidationFunction_ContractOwner((address,uint256,bytes,bytes,uint256,uint256,uint256,uint256,uint256,bytes,bytes)) (runs: 256, μ: 130908, ~: 130897) MultiOwnerPluginTest:testFuzz_userOpValidationFunction_ContractOwnerWithEOAOwner((address,uint256,bytes,bytes,uint256,uint256,uint256,uint256,uint256,bytes,bytes)) (runs: 256, μ: 144523, ~: 144512) MultiOwnerPluginTest:testFuzz_userOpValidationFunction_EOAOwner(string,(address,uint256,bytes,bytes,uint256,uint256,uint256,uint256,uint256,bytes,bytes)) (runs: 256, μ: 138777, ~: 138780) -MultiOwnerPluginTest:testFuzz_userOpValidationFunction_PasskeyOwner((address,uint256,bytes,bytes,uint256,uint256,uint256,uint256,uint256,bytes,bytes)) (runs: 256, μ: 373604, ~: 373579) +MultiOwnerPluginTest:testFuzz_userOpValidationFunction_PasskeyOwner((address,uint256,bytes,bytes,uint256,uint256,uint256,uint256,uint256,bytes,bytes)) (runs: 256, μ: 373571, ~: 373577) MultiOwnerPluginTest:test_eip712Domain() (gas: 35438) -MultiOwnerPluginTest:test_isValidSignature_failMalformedAddress() (gas: 15610) +MultiOwnerPluginTest:test_isValidSignature_failMalformedAddress() (gas: 15544) MultiOwnerPluginTest:test_isValidSignature_failWithOutOfBounds() (gas: 12319) MultiOwnerPluginTest:test_multiOwnerPlugin_sentinelIsNotOwner() (gas: 19805) MultiOwnerPluginTest:test_onInstall_failWithEmptyOwners() (gas: 36299) @@ -27,12 +27,13 @@ MultiOwnerPluginTest:test_pluginManifest() (gas: 38695) MultiOwnerPluginTest:test_pluginMetadata_success() (gas: 16954) MultiOwnerPluginTest:test_runtimeValidationFunction_OwnerOrSelf() (gas: 26703) MultiOwnerPluginTest:test_updateOwnersPublicKeys_failWithInvalidAddress() (gas: 55729) +MultiOwnerPluginTest:test_updateOwners_addAndRemove() (gas: 179888) MultiOwnerPluginTest:test_updateOwners_failWithDuplicatedAddresses() (gas: 85516) -MultiOwnerPluginTest:test_updateOwners_failWithEmptyOwners() (gas: 70711) +MultiOwnerPluginTest:test_updateOwners_failWithEmptyOwners() (gas: 70726) MultiOwnerPluginTest:test_updateOwners_failWithLimitExceeded() (gas: 1924832) MultiOwnerPluginTest:test_updateOwners_failWithNotExist() (gas: 58523) MultiOwnerPluginTest:test_updateOwners_failWithZeroAddressOwner() (gas: 56428) -MultiOwnerPluginTest:test_updateOwners_success() (gas: 119826) +MultiOwnerPluginTest:test_updateOwners_success() (gas: 119812) WebauthnModularAccountFactoryTest:test_2StepOwnershipTransfer() (gas: 87593) WebauthnModularAccountFactoryTest:test_addStake() (gas: 106153) WebauthnModularAccountFactoryTest:test_addressMatch() (gas: 801814) diff --git a/src/WebauthnOwnerPlugin.sol b/src/WebauthnOwnerPlugin.sol index 521cf84..b9b082b 100644 --- a/src/WebauthnOwnerPlugin.sol +++ b/src/WebauthnOwnerPlugin.sol @@ -71,8 +71,15 @@ contract WebauthnOwnerPlugin is BasePlugin, IWebauthnOwnerPlugin, IERC1271 { for (uint256 removeIndex = 0; removeIndex < ownersToRemove.length; ++removeIndex) { uint256 ownerIndex = keys.find(ownersToRemove[removeIndex], ownerCount); if (ownerIndex == type(uint256).max) revert OwnerDoesNotExist(ownersToRemove[removeIndex].toAddress()); - if (--ownerCount == 0) break; - keys[ownerIndex] = addIndex < ownersToAdd.length ? ownersToAdd[addIndex++] : keys[ownerCount]; + if (addIndex < ownersToAdd.length) { + if (ownersToAdd[addIndex].isInvalid() || keys.contains(ownersToAdd[addIndex], ownerCount)) { + revert InvalidOwner(ownersToAdd[addIndex].toAddress()); + } + keys[ownerIndex] = ownersToAdd[addIndex++]; + } else { + if (--ownerCount == 0) break; + keys[ownerIndex] = keys[ownerCount]; + } owners.publicKeys[ownerIndex] = keys[ownerIndex]; } for (; addIndex < ownersToAdd.length; ++addIndex) { diff --git a/test/WebauthnOwnerPlugin.t.sol b/test/WebauthnOwnerPlugin.t.sol index 4a5698f..89c115d 100644 --- a/test/WebauthnOwnerPlugin.t.sol +++ b/test/WebauthnOwnerPlugin.t.sol @@ -212,6 +212,29 @@ contract MultiOwnerPluginTest is Test { assertEq(newOwnerList[0], owner3); } + function test_updateOwners_addAndRemove() external { + (address[] memory owners) = plugin.ownersOf(accountA); + assertEq(ownerArray, owners); + + address[] memory ownersToRemove = new address[](3); + ownersToRemove[0] = owner2; + ownersToRemove[1] = owner3; + ownersToRemove[2] = owner1; + + address[] memory ownersToAdd = new address[](2); + ownersToAdd[0] = address(1); + + vm.expectRevert(abi.encodeWithSelector(IMultiOwnerPlugin.InvalidOwner.selector, address(0))); + plugin.updateOwners(ownersToAdd, ownersToRemove); + + ownersToAdd[1] = address(2); + plugin.updateOwners(ownersToAdd, ownersToRemove); + + (address[] memory newOwnerList) = plugin.ownersOf(accountA); + assertEq(newOwnerList.length, 2); + assertEq(newOwnerList, ownersToAdd); + } + function test_updateOwners_failWithNotExist() external { address[] memory ownersToRemove = new address[](1); ownersToRemove[0] = address(contractOwner);