From 582006f651d984d3e4efc464ed30ccb4244928ed Mon Sep 17 00:00:00 2001 From: danilo neves cruz Date: Sat, 29 Jun 2024 18:22:51 -0300 Subject: [PATCH] =?UTF-8?q?=E2=9C=85=20test:=20increase=20code=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .gas-snapshot | 74 ++++++++++++++---------- src/WebauthnOwnerPlugin.sol | 12 ++-- test/WebauthnModularAccountFactory.t.sol | 39 ++++++++++++- test/WebauthnOwnerPlugin.t.sol | 71 ++++++++++++++++++++++- 4 files changed, 155 insertions(+), 41 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index 3a61111..1445c29 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -2,43 +2,53 @@ MultiOwnerPluginIntegration:test_ownerPlugin_successInstallation() (gas: 39135) MultiOwnerPluginIntegration:test_runtimeValidation_alwaysAllow_isValidSignature() (gas: 104093) MultiOwnerPluginIntegration:test_runtimeValidation_ownerOrSelf_standardExecute() (gas: 145057) MultiOwnerPluginIntegration:test_userOpValidation_owner_standardExecute() (gas: 347849) -MultiOwnerPluginTest:testFuzz_isValidSignature_ContractOwner(bytes32) (runs: 256, μ: 110118, ~: 110118) -MultiOwnerPluginTest:testFuzz_isValidSignature_ContractOwnerWithEOAOwner(bytes32) (runs: 256, μ: 120437, ~: 120437) -MultiOwnerPluginTest:testFuzz_isValidSignature_EOAOwner(string,bytes32) (runs: 256, μ: 130754, ~: 130747) -MultiOwnerPluginTest:testFuzz_isValidSignature_PasskeyOwner(bytes32) (runs: 256, μ: 365635, ~: 365718) -MultiOwnerPluginTest:testFuzz_userOpValidationFunction_ContractOwner((address,uint256,bytes,bytes,uint256,uint256,uint256,uint256,uint256,bytes,bytes)) (runs: 256, μ: 130892, ~: 130881) -MultiOwnerPluginTest:testFuzz_userOpValidationFunction_ContractOwnerWithEOAOwner((address,uint256,bytes,bytes,uint256,uint256,uint256,uint256,uint256,bytes,bytes)) (runs: 256, μ: 144491, ~: 144480) -MultiOwnerPluginTest:testFuzz_userOpValidationFunction_EOAOwner(string,(address,uint256,bytes,bytes,uint256,uint256,uint256,uint256,uint256,bytes,bytes)) (runs: 256, μ: 138767, ~: 138770) -MultiOwnerPluginTest:testFuzz_userOpValidationFunction_PasskeyOwner((address,uint256,bytes,bytes,uint256,uint256,uint256,uint256,uint256,bytes,bytes)) (runs: 256, μ: 373522, ~: 373350) -MultiOwnerPluginTest:test_eip712Domain() (gas: 35394) -MultiOwnerPluginTest:test_multiOwnerPlugin_sentinelIsNotOwner() (gas: 19794) -MultiOwnerPluginTest:test_onInstall_failWithInvalidAddress() (gas: 38434) -MultiOwnerPluginTest:test_onInstall_failWithLimitExceeded() (gas: 1714334) -MultiOwnerPluginTest:test_onInstall_success() (gas: 94764) +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, μ: 365560, ~: 365612) +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, μ: 373774, ~: 373608) +MultiOwnerPluginTest:test_eip712Domain() (gas: 35438) +MultiOwnerPluginTest:test_isValidSignature_failMalformedAddress() (gas: 15610) +MultiOwnerPluginTest:test_isValidSignature_failWithOutOfBounds() (gas: 12319) +MultiOwnerPluginTest:test_multiOwnerPlugin_sentinelIsNotOwner() (gas: 19805) +MultiOwnerPluginTest:test_onInstall_failWithEmptyOwners() (gas: 36299) +MultiOwnerPluginTest:test_onInstall_failWithInvalidAddress() (gas: 38500) +MultiOwnerPluginTest:test_onInstall_failWithLimitExceeded() (gas: 1714289) +MultiOwnerPluginTest:test_onInstall_success() (gas: 94741) MultiOwnerPluginTest:test_onUninstall_success() (gas: 84935) -MultiOwnerPluginTest:test_pluginInitializeGuards() (gas: 163889) -MultiOwnerPluginTest:test_pluginManifest() (gas: 38747) -MultiOwnerPluginTest:test_runtimeValidationFunction_OwnerOrSelf() (gas: 26747) -MultiOwnerPluginTest:test_updateOwnersPublicKeys_failWithInvalidAddress() (gas: 55749) -MultiOwnerPluginTest:test_updateOwners_failWithDuplicatedAddresses() (gas: 85472) -MultiOwnerPluginTest:test_updateOwners_failWithEmptyOwners() (gas: 70734) +MultiOwnerPluginTest:test_ownerIndexOf_failWithNotExist() (gas: 28310) +MultiOwnerPluginTest:test_pluginInitializeGuards() (gas: 163933) +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_failWithDuplicatedAddresses() (gas: 85516) +MultiOwnerPluginTest:test_updateOwners_failWithEmptyOwners() (gas: 70711) MultiOwnerPluginTest:test_updateOwners_failWithLimitExceeded() (gas: 1924832) -MultiOwnerPluginTest:test_updateOwners_failWithNotExist() (gas: 58568) -MultiOwnerPluginTest:test_updateOwners_failWithZeroAddressOwner() (gas: 56362) +MultiOwnerPluginTest:test_updateOwners_failWithNotExist() (gas: 58523) +MultiOwnerPluginTest:test_updateOwners_failWithZeroAddressOwner() (gas: 56428) MultiOwnerPluginTest:test_updateOwners_success() (gas: 119826) -WebauthnModularAccountFactoryTest:test_2StepOwnershipTransfer() (gas: 87560) -WebauthnModularAccountFactoryTest:test_addStake() (gas: 106151) -WebauthnModularAccountFactoryTest:test_addressMatch() (gas: 801803) +WebauthnModularAccountFactoryTest:test_2StepOwnershipTransfer() (gas: 87593) +WebauthnModularAccountFactoryTest:test_addStake() (gas: 106153) +WebauthnModularAccountFactoryTest:test_addressMatch() (gas: 801814) WebauthnModularAccountFactoryTest:test_badOwnersArray() (gas: 18409) -WebauthnModularAccountFactoryTest:test_deploy() (gas: 810337) -WebauthnModularAccountFactoryTest:test_deployCollision() (gas: 825988) +WebauthnModularAccountFactoryTest:test_constructor_failWithZeroAddress() (gas: 1740649) +WebauthnModularAccountFactoryTest:test_deploy() (gas: 810381) +WebauthnModularAccountFactoryTest:test_deployCollision() (gas: 826021) WebauthnModularAccountFactoryTest:test_deployWithDuplicateOwners() (gas: 737357) WebauthnModularAccountFactoryTest:test_deployWithUnsortedOwners() (gas: 737379) -WebauthnModularAccountFactoryTest:test_deploy_PasskeyOwner() (gas: 797170) +WebauthnModularAccountFactoryTest:test_deploy_PasskeyOwner() (gas: 797192) WebauthnModularAccountFactoryTest:test_deployedAccountHasCorrectPlugins() (gas: 801483) -WebauthnModularAccountFactoryTest:test_getAddressWithMaxOwnersAndDeploy() (gas: 2704020) -WebauthnModularAccountFactoryTest:test_getAddressWithTooManyOwners() (gas: 205715) -WebauthnModularAccountFactoryTest:test_getAddressWithUnsortedOwners() (gas: 11320) -WebauthnModularAccountFactoryTest:test_unlockStake() (gas: 147811) +WebauthnModularAccountFactoryTest:test_getAddressWithMaxOwnersAndDeploy() (gas: 2704053) +WebauthnModularAccountFactoryTest:test_getAddressWithTooManyOwners() (gas: 205692) +WebauthnModularAccountFactoryTest:test_getAddressWithUnsortedOwners() (gas: 11342) +WebauthnModularAccountFactoryTest:test_renounceOwnership() (gas: 10459) +WebauthnModularAccountFactoryTest:test_unlockStake() (gas: 147882) WebauthnModularAccountFactoryTest:test_withdraw() (gas: 128078) -WebauthnModularAccountFactoryTest:test_withdrawStake() (gas: 210368) \ No newline at end of file +WebauthnModularAccountFactoryTest:test_withdrawStake() (gas: 210351) +WebauthnModularAccountFactoryTest:test_withdraw_token() (gas: 995155) \ No newline at end of file diff --git a/src/WebauthnOwnerPlugin.sol b/src/WebauthnOwnerPlugin.sol index d959405..69926a4 100644 --- a/src/WebauthnOwnerPlugin.sol +++ b/src/WebauthnOwnerPlugin.sol @@ -70,15 +70,17 @@ contract WebauthnOwnerPlugin is BasePlugin, IWebauthnOwnerPlugin, IERC1271 { uint256 addIndex = 0; 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 ( + ownerIndex == type(uint256).max // + ) revert OwnerDoesNotExist(ownersToRemove[removeIndex].toAddress()); if (--ownerCount == 0) break; keys[ownerIndex] = addIndex < ownersToAdd.length ? ownersToAdd[addIndex++] : keys[ownerCount]; owners.publicKeys[ownerIndex] = keys[ownerIndex]; } for (; addIndex < ownersToAdd.length; ++addIndex) { - if (ownersToAdd[addIndex].isInvalid() || keys.contains(ownersToAdd[addIndex], ownerCount)) { - revert InvalidOwner(ownersToAdd[addIndex].toAddress()); - } + if ( + ownersToAdd[addIndex].isInvalid() || keys.contains(ownersToAdd[addIndex], ownerCount) // + ) revert InvalidOwner(ownersToAdd[addIndex].toAddress()); keys[ownerCount] = ownersToAdd[addIndex]; owners.publicKeys[ownerCount] = keys[ownerCount]; ++ownerCount; @@ -327,7 +329,7 @@ contract WebauthnOwnerPlugin is BasePlugin, IWebauthnOwnerPlugin, IERC1271 { PublicKey memory owner = _owners[account].get(uint8(signature[0])); if (owner.y == 0) { - if (owner.x > type(uint160).max) revert InvalidEthereumAddressOwner(bytes32(owner.x)); + if (owner.x > type(uint160).max) revert InvalidEthereumAddressOwner(bytes32(owner.x)); // should be impossible return address(uint160(owner.x)).isValidSignatureNowCalldata(message, signature[1:]); } diff --git a/test/WebauthnModularAccountFactory.t.sol b/test/WebauthnModularAccountFactory.t.sol index 8240936..657a65c 100644 --- a/test/WebauthnModularAccountFactory.t.sol +++ b/test/WebauthnModularAccountFactory.t.sol @@ -7,12 +7,16 @@ import { EntryPoint } from "account-abstraction/core/EntryPoint.sol"; import { PluginManagerInternals } from "modular-account/src/account/PluginManagerInternals.sol"; import { UpgradeableModularAccount } from "modular-account/src/account/UpgradeableModularAccount.sol"; +import { IEntryPoint } from "modular-account/src/interfaces/erc4337/IEntryPoint.sol"; +import { MockERC20 } from "solady/../test/utils/mocks/MockERC20.sol"; import { ECDSA } from "solady/utils/ECDSA.sol"; import { DeployScript } from "../script/Deploy.s.sol"; import { OwnersLib } from "../src/OwnersLib.sol"; -import { OwnersLimitExceeded, WebauthnModularAccountFactory } from "../src/WebauthnModularAccountFactory.sol"; +import { + InvalidAction, OwnersLimitExceeded, WebauthnModularAccountFactory +} from "../src/WebauthnModularAccountFactory.sol"; import { IMultiOwnerPlugin, MAX_OWNERS, PublicKey, WebauthnOwnerPlugin } from "../src/WebauthnOwnerPlugin.sol"; // solhint-disable func-name-mixedcase @@ -161,6 +165,14 @@ contract WebauthnModularAccountFactoryTest is Test { assertEq(address(factory).balance, 0); } + function test_withdraw_token() external { + MockERC20 token = new MockERC20("A", "A", 18); + token.mint(address(factory), 1); + assertEq(token.balanceOf(address(factory)), 1); + factory.withdraw(payable(address(this)), address(token), 1); + assertEq(token.balanceOf(address(factory)), 0); + } + function test_2StepOwnershipTransfer() external { assertEq(factory.owner(), address(this)); factory.transferOwnership(owner1); @@ -218,6 +230,31 @@ contract WebauthnModularAccountFactoryTest is Test { factory.createAccount(0, tempOwners.toPublicKeys()); } + function test_renounceOwnership() external { + vm.expectRevert(InvalidAction.selector); + factory.renounceOwnership(); + } + + function test_constructor_failWithZeroAddress() external { + address impl = factory.IMPL(); + bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); + + vm.expectRevert(InvalidAction.selector); + new WebauthnModularAccountFactory(address(this), address(0), impl, manifestHash, IEntryPoint(address(entryPoint))); + + vm.expectRevert(InvalidAction.selector); + new WebauthnModularAccountFactory( + address(this), address(plugin), address(0), manifestHash, IEntryPoint(address(entryPoint)) + ); + + vm.expectRevert(InvalidAction.selector); + new WebauthnModularAccountFactory(address(this), address(plugin), impl, manifestHash, IEntryPoint(address(0))); + + new WebauthnModularAccountFactory( + address(this), address(plugin), impl, manifestHash, IEntryPoint(address(entryPoint)) + ); + } + // to receive funds from withdraw receive() external payable { } } diff --git a/test/WebauthnOwnerPlugin.t.sol b/test/WebauthnOwnerPlugin.t.sol index 9d423af..4a5698f 100644 --- a/test/WebauthnOwnerPlugin.t.sol +++ b/test/WebauthnOwnerPlugin.t.sol @@ -7,12 +7,12 @@ import { EntryPoint } from "account-abstraction/core/EntryPoint.sol"; import { UpgradeableModularAccount } from "modular-account/src/account/UpgradeableModularAccount.sol"; import { IEntryPoint } from "modular-account/src/interfaces/erc4337/IEntryPoint.sol"; -import { BasePlugin } from "modular-account/src/plugins/BasePlugin.sol"; import { IMultiOwnerPlugin } from "modular-account/src/plugins/owner/IMultiOwnerPlugin.sol"; import { ContractOwner } from "modular-account/test/mocks/ContractOwner.sol"; -import { PluginManifest } from "modular-account-libs/interfaces/IPlugin.sol"; +import { PluginManifest, PluginMetadata } from "modular-account-libs/interfaces/IPlugin.sol"; import { UserOperation } from "modular-account-libs/interfaces/UserOperation.sol"; +import { BasePlugin } from "modular-account-libs/plugins/BasePlugin.sol"; import { ECDSA } from "solady/utils/ECDSA.sol"; @@ -20,7 +20,7 @@ import { Utils, WebAuthnInfo } from "webauthn-sol/../test/Utils.sol"; import { WebAuthn } from "webauthn-sol/WebAuthn.sol"; import { DeployScript } from "../script/Deploy.s.sol"; -import { OwnersLib } from "../src/OwnersLib.sol"; +import { IndexOutOfBounds, OwnersLib } from "../src/OwnersLib.sol"; import { IWebauthnOwnerPlugin, MAX_OWNERS, PublicKey, WebauthnOwnerPlugin } from "../src/WebauthnOwnerPlugin.sol"; import { TestLib } from "./utils/TestLib.sol"; @@ -129,6 +129,13 @@ contract MultiOwnerPluginTest is Test { vm.stopPrank(); } + function test_onInstall_failWithEmptyOwners() external { + vm.expectRevert(IMultiOwnerPlugin.EmptyOwnersNotAllowed.selector); + vm.startPrank(address(contractOwner)); + plugin.onInstall(abi.encode(new PublicKey[](0))); + vm.stopPrank(); + } + function test_onInstall_failWithLimitExceeded() external { address[] memory owners = new address[](MAX_OWNERS + 1); for (uint160 i = 0; i < owners.length; ++i) { @@ -308,6 +315,37 @@ contract MultiOwnerPluginTest is Test { assertEq(_1271_MAGIC_VALUE, plugin.isValidSignature(digest, signature)); } + function test_isValidSignature_failWithOutOfBounds() external { + vm.expectRevert(IndexOutOfBounds.selector); + plugin.isValidSignature("", abi.encodePacked(uint8(4), "")); + } + + function test_isValidSignature_failMalformedAddress() external { + vm.store( + address(plugin), + bytes32(uint256(keccak256(abi.encode(accountA, uint256(0)))) + 1), + bytes32(uint256(type(uint160).max) + 1) + ); + + vm.expectRevert( + abi.encodeWithSelector( + IWebauthnOwnerPlugin.InvalidEthereumAddressOwner.selector, bytes32(uint256(type(uint160).max) + 1) + ) + ); + plugin.isValidSignature("", abi.encodePacked(uint8(0), "")); + } + + function testFuzz_runtimeValidationFunction_BadFunctionId(uint8 functionId) external { + vm.assume(functionId != uint8(IMultiOwnerPlugin.FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF)); + + vm.expectRevert( + abi.encodeWithSelector( + BasePlugin.NotImplemented.selector, BasePlugin.runtimeValidationFunction.selector, functionId + ) + ); + plugin.runtimeValidationFunction(functionId, address(0), 0, ""); + } + function test_runtimeValidationFunction_OwnerOrSelf() external { // should pass with owner as sender plugin.runtimeValidationFunction( @@ -432,6 +470,23 @@ contract MultiOwnerPluginTest is Test { assertEq(resSuccess, 0); } + function testFuzz_userOpValidationFunction_BadFunctionId(uint8 functionId) external { + vm.assume(functionId != uint8(IMultiOwnerPlugin.FunctionId.USER_OP_VALIDATION_OWNER)); + + UserOperation memory userOp; + vm.expectRevert( + abi.encodeWithSelector( + BasePlugin.NotImplemented.selector, BasePlugin.userOpValidationFunction.selector, functionId + ) + ); + plugin.userOpValidationFunction(functionId, userOp, 0); + } + + function test_ownerIndexOf_failWithNotExist() external { + vm.expectRevert(abi.encodeWithSelector(IMultiOwnerPlugin.OwnerDoesNotExist.selector, address(0xb0b))); + plugin.ownerIndexOf(accountA, address(0xb0b).toPublicKey()); + } + function test_pluginInitializeGuards() external { plugin.onUninstall(bytes("")); @@ -447,4 +502,14 @@ contract MultiOwnerPluginTest is Test { vm.expectRevert(abi.encodeWithSelector(BasePlugin.AlreadyInitialized.selector)); plugin.onInstall(abi.encode(addrArr, new address[](0))); } + + function test_pluginMetadata_success() external view { + PluginMetadata memory metadata = plugin.pluginMetadata(); + + assertEq(metadata.permissionDescriptors.length, 2); + assertEq(metadata.permissionDescriptors[0].permissionDescription, "Modify Ownership"); + assertEq(metadata.permissionDescriptors[1].permissionDescription, "Modify Ownership"); + assertEq(metadata.permissionDescriptors[0].functionSelector, IMultiOwnerPlugin.updateOwners.selector); + assertEq(metadata.permissionDescriptors[1].functionSelector, IWebauthnOwnerPlugin.updateOwnersPublicKeys.selector); + } }