🐛 plugin: fix count and validation on owner update

addresses quantstamp EXA-1
exactly · Jul 2, 2024 · 6992ecd · 6992ecd
1 parent 395368d
commit 6992ecd
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 7 deletions.
diff --git 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
@@ -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
@@ -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);