From 1987ab55e6d9b4588ac02ed12f8dd5ca411da9b7 Mon Sep 17 00:00:00 2001 From: adam Date: Tue, 15 Oct 2024 17:40:43 -0400 Subject: [PATCH] feat: sizes script test: expand test coverage for MABase refactor: remove duplicate error definitions refactor: simplify error types in MABase feat: custom errors for all module functions feat: change error param to moduleentity fix: reorder errors style: fix import formatting --- ...t_Runtime_UseSessionKey_Case1_Counter.snap | 2 +- ...unt_Runtime_UseSessionKey_Case1_Token.snap | 2 +- .../ModularAccount_UserOp_BatchTransfers.snap | 2 +- .../ModularAccount_UserOp_Erc20Transfer.snap | 2 +- ...ccount_UserOp_InstallSessionKey_Case1.snap | 2 +- .../ModularAccount_UserOp_NativeTransfer.snap | 2 +- ...nt_UserOp_UseSessionKey_Case1_Counter.snap | 2 +- ...ount_UserOp_UseSessionKey_Case1_Token.snap | 2 +- ...ularAccount_UserOp_deferredValidation.snap | 2 +- ...t_Runtime_UseSessionKey_Case1_Counter.snap | 2 +- ...unt_Runtime_UseSessionKey_Case1_Token.snap | 2 +- ...iModularAccount_UserOp_BatchTransfers.snap | 2 +- ...miModularAccount_UserOp_Erc20Transfer.snap | 2 +- ...ccount_UserOp_InstallSessionKey_Case1.snap | 2 +- ...iModularAccount_UserOp_NativeTransfer.snap | 2 +- ...nt_UserOp_UseSessionKey_Case1_Counter.snap | 2 +- ...ount_UserOp_UseSessionKey_Case1_Token.snap | 2 +- ...ularAccount_UserOp_deferredValidation.snap | 2 +- package.json | 1 + src/account/ModularAccountBase.sol | 32 ++- src/libraries/ExecutionLib.sol | 197 ++++++++++++------ test/account/GlobalValidationTest.t.sol | 39 ++++ test/account/ModularAccount.t.sol | 82 +++++++- test/account/MultiValidation.t.sol | 6 +- test/account/PerHookData.t.sol | 50 +++-- test/account/UOCallBuffer.t.sol | 10 +- test/account/ValidationAssocHooks.t.sol | 42 +++- test/account/ValidationIntersection.t.sol | 5 +- test/mocks/MockInterface.sol | 6 + test/mocks/MockRevertingConstructor.sol | 8 + test/modules/AllowlistModule.t.sol | 30 ++- test/modules/NativeTokenLimitModule.t.sol | 26 ++- test/modules/PaymasterGuardModule.t.sol | 9 +- test/modules/TimeRangeModule.t.sol | 19 +- 34 files changed, 441 insertions(+), 157 deletions(-) create mode 100644 test/mocks/MockInterface.sol create mode 100644 test/mocks/MockRevertingConstructor.sol diff --git a/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Counter.snap b/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Counter.snap index ea6db0041..4cac3e2b4 100644 --- a/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Counter.snap +++ b/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Counter.snap @@ -1 +1 @@ -79531 \ No newline at end of file +79501 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Token.snap b/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Token.snap index c924bddf5..653e69286 100644 --- a/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Token.snap +++ b/.forge-snapshots/ModularAccount_Runtime_UseSessionKey_Case1_Token.snap @@ -1 +1 @@ -113089 \ No newline at end of file +113059 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_BatchTransfers.snap b/.forge-snapshots/ModularAccount_UserOp_BatchTransfers.snap index f8918f329..531657a56 100644 --- a/.forge-snapshots/ModularAccount_UserOp_BatchTransfers.snap +++ b/.forge-snapshots/ModularAccount_UserOp_BatchTransfers.snap @@ -1 +1 @@ -198425 \ No newline at end of file +198437 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_Erc20Transfer.snap b/.forge-snapshots/ModularAccount_UserOp_Erc20Transfer.snap index 62aded4ee..1c39110fb 100644 --- a/.forge-snapshots/ModularAccount_UserOp_Erc20Transfer.snap +++ b/.forge-snapshots/ModularAccount_UserOp_Erc20Transfer.snap @@ -1 +1 @@ -185394 \ No newline at end of file +185406 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_InstallSessionKey_Case1.snap b/.forge-snapshots/ModularAccount_UserOp_InstallSessionKey_Case1.snap index fb49cc973..0ac264d7e 100644 --- a/.forge-snapshots/ModularAccount_UserOp_InstallSessionKey_Case1.snap +++ b/.forge-snapshots/ModularAccount_UserOp_InstallSessionKey_Case1.snap @@ -1 +1 @@ -532162 \ No newline at end of file +532174 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_NativeTransfer.snap b/.forge-snapshots/ModularAccount_UserOp_NativeTransfer.snap index 436151f5b..d60bdac3e 100644 --- a/.forge-snapshots/ModularAccount_UserOp_NativeTransfer.snap +++ b/.forge-snapshots/ModularAccount_UserOp_NativeTransfer.snap @@ -1 +1 @@ -161675 \ No newline at end of file +161687 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Counter.snap b/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Counter.snap index a4f948667..96000a801 100644 --- a/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Counter.snap +++ b/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Counter.snap @@ -1 +1 @@ -195886 \ No newline at end of file +195938 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Token.snap b/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Token.snap index 7b1c42ae7..13ac764ba 100644 --- a/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Token.snap +++ b/.forge-snapshots/ModularAccount_UserOp_UseSessionKey_Case1_Token.snap @@ -1 +1 @@ -227010 \ No newline at end of file +227062 \ No newline at end of file diff --git a/.forge-snapshots/ModularAccount_UserOp_deferredValidation.snap b/.forge-snapshots/ModularAccount_UserOp_deferredValidation.snap index 42d0bbdaa..9b33cc6f0 100644 --- a/.forge-snapshots/ModularAccount_UserOp_deferredValidation.snap +++ b/.forge-snapshots/ModularAccount_UserOp_deferredValidation.snap @@ -1 +1 @@ -257385 \ No newline at end of file +257407 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Counter.snap b/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Counter.snap index da9056908..3e62ef7e7 100644 --- a/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Counter.snap +++ b/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Counter.snap @@ -1 +1 @@ -79969 \ No newline at end of file +79939 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Token.snap b/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Token.snap index eb30ea85a..b8748c0c8 100644 --- a/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Token.snap +++ b/.forge-snapshots/SemiModularAccount_Runtime_UseSessionKey_Case1_Token.snap @@ -1 +1 @@ -113527 \ No newline at end of file +113497 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_BatchTransfers.snap b/.forge-snapshots/SemiModularAccount_UserOp_BatchTransfers.snap index 8b587a17c..b12e3ecdd 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_BatchTransfers.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_BatchTransfers.snap @@ -1 +1 @@ -193634 \ No newline at end of file +193639 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_Erc20Transfer.snap b/.forge-snapshots/SemiModularAccount_UserOp_Erc20Transfer.snap index e216c182b..ba360feea 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_Erc20Transfer.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_Erc20Transfer.snap @@ -1 +1 @@ -180730 \ No newline at end of file +180735 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_InstallSessionKey_Case1.snap b/.forge-snapshots/SemiModularAccount_UserOp_InstallSessionKey_Case1.snap index a9d2d53a1..d8dc983aa 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_InstallSessionKey_Case1.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_InstallSessionKey_Case1.snap @@ -1 +1 @@ -529742 \ No newline at end of file +529747 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_NativeTransfer.snap b/.forge-snapshots/SemiModularAccount_UserOp_NativeTransfer.snap index f764a4201..b10db97ae 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_NativeTransfer.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_NativeTransfer.snap @@ -1 +1 @@ -157005 \ No newline at end of file +157010 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Counter.snap b/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Counter.snap index 7bf25d208..a58597c73 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Counter.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Counter.snap @@ -1 +1 @@ -196127 \ No newline at end of file +196232 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Token.snap b/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Token.snap index dd92cd6ee..f57c5ed58 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Token.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_UseSessionKey_Case1_Token.snap @@ -1 +1 @@ -227263 \ No newline at end of file +227368 \ No newline at end of file diff --git a/.forge-snapshots/SemiModularAccount_UserOp_deferredValidation.snap b/.forge-snapshots/SemiModularAccount_UserOp_deferredValidation.snap index 176ac6e84..69ad057ec 100644 --- a/.forge-snapshots/SemiModularAccount_UserOp_deferredValidation.snap +++ b/.forge-snapshots/SemiModularAccount_UserOp_deferredValidation.snap @@ -1 +1 @@ -254003 \ No newline at end of file +254026 \ No newline at end of file diff --git a/package.json b/package.json index 29f8a28c2..089c9a36a 100644 --- a/package.json +++ b/package.json @@ -24,6 +24,7 @@ "lint:gas": "solhint --max-warnings 0 -c ./config/solhint-gas.json './gas/**/*.sol'", "lint:script": "solhint --max-warnings 0 -c ./config/solhint-script.json './script/**/*.sol'", "prep": "pnpm fmt && forge b --deny-warnings && pnpm lint && pnpm test && pnpm gas", + "sizes": "FOUNDRY_PROFILE=optimized-build forge b --sizes | grep '^|' | grep -v -e '| 17 |' -e 'Lib'", "test": "forge test" } } diff --git a/src/account/ModularAccountBase.sol b/src/account/ModularAccountBase.sol index ca1167109..e93fb529b 100644 --- a/src/account/ModularAccountBase.sol +++ b/src/account/ModularAccountBase.sol @@ -87,20 +87,16 @@ abstract contract ModularAccountBase is event DeferredActionNonceInvalidated(uint256 nonce); - error PostExecHookReverted(address module, uint32 entityId, bytes revertReason); - error PreExecHookReverted(address module, uint32 entityId, bytes revertReason); - error PreRuntimeValidationHookFailed(address module, uint32 entityId, bytes revertReason); + error CreateFailed(); + error DeferredActionNonceInvalid(); + error DeferredActionSignatureInvalid(); error RequireUserOperationContext(); - error RuntimeValidationFunctionReverted(address module, uint32 entityId, bytes revertReason); error SelfCallRecursionDepthExceeded(); - error SignatureValidationInvalid(address module, uint32 entityId); - error UserOpValidationInvalid(address module, uint32 entityId); - error UnexpectedAggregator(address module, uint32 entityId, address aggregator); + error SignatureValidationInvalid(ModuleEntity validationFunction); + error UserOpValidationInvalid(ModuleEntity validationFunction); + error UnexpectedAggregator(ModuleEntity validationFunction, address aggregator); error UnrecognizedFunction(bytes4 selector); error ValidationFunctionMissing(bytes4 selector); - error DeferredActionNonceInvalid(); - error DeferredActionSignatureInvalid(); - error CreateFailed(); // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installExecution, uninstallExecution, @@ -574,14 +570,14 @@ abstract contract ModularAccountBase is (currentSignatureSlice, signature) = signature.advanceSegmentIfAtIndex(uint8(preUserOpValidationHooks.length - i - 1)); - uint256 currentValidationRes = ExecutionLib.invokeUserOpCallBuffer( - userOpCallBuffer, preUserOpValidationHooks[i].moduleEntity(), currentSignatureSlice - ); + ModuleEntity uoValidationHook = preUserOpValidationHooks[i].moduleEntity(); + + uint256 currentValidationRes = + ExecutionLib.invokeUserOpCallBuffer(userOpCallBuffer, uoValidationHook, currentSignatureSlice); if (uint160(currentValidationRes) > 1) { // If the aggregator is not 0 or 1, it is an unexpected value - (address module, uint32 entityId) = preUserOpValidationHooks[i].moduleEntity().unpack(); - revert UnexpectedAggregator(module, entityId, address(uint160(currentValidationRes))); + revert UnexpectedAggregator(uoValidationHook, address(uint160(currentValidationRes))); } validationRes = _coalescePreValidation(validationRes, currentValidationRes); } @@ -727,8 +723,7 @@ abstract contract ModularAccountBase is AccountStorage storage _storage = getAccountStorage(); if (!_storage.validationStorage[userOpValidationFunction].isUserOpValidation) { - (address module, uint32 entityId) = userOpValidationFunction.unpack(); - revert UserOpValidationInvalid(module, entityId); + revert UserOpValidationInvalid(userOpValidationFunction); } ExecutionLib.convertToValidationBuffer(callBuffer); @@ -786,8 +781,7 @@ abstract contract ModularAccountBase is AccountStorage storage _storage = getAccountStorage(); if (!_storage.validationStorage[sigValidation].isSignatureValidation) { - (address module, uint32 entityId) = sigValidation.unpack(); - revert SignatureValidationInvalid(module, entityId); + revert SignatureValidationInvalid(sigValidation); } if (ExecutionLib.invokeSignatureValidation(buffer, sigValidation, signatureSegment) == _1271_MAGIC_VALUE) { diff --git a/src/libraries/ExecutionLib.sol b/src/libraries/ExecutionLib.sol index 3b869856f..f5b2b665e 100644 --- a/src/libraries/ExecutionLib.sol +++ b/src/libraries/ExecutionLib.sol @@ -27,11 +27,14 @@ using HookConfigLib for HookConfig; // Functions are more readable in original order // solhint-disable ordering library ExecutionLib { - // Duplicate definition to make it easier to revert in the library. - error PostExecHookReverted(address module, uint32 entityId, bytes revertReason); - error PreExecHookReverted(address module, uint32 entityId, bytes revertReason); - error PreRuntimeValidationHookFailed(address module, uint32 entityId, bytes revertReason); - error RuntimeValidationFunctionReverted(address module, uint32 entityId, bytes revertReason); + error PostExecHookReverted(ModuleEntity moduleFunction, bytes revertReason); + error PreExecHookReverted(ModuleEntity moduleFunction, bytes revertReason); + error PreRuntimeValidationHookReverted(ModuleEntity moduleFunction, bytes revertReason); + error PreSignatureValidationHookReverted(ModuleEntity moduleFunction, bytes revertReason); + error PreUserOpValidationHookReverted(ModuleEntity moduleFunction, bytes revertReason); + error RuntimeValidationFunctionReverted(ModuleEntity moduleFunction, bytes revertReason); + error SignatureValidationFunctionReverted(ModuleEntity moduleFunction, bytes revertReason); + error UserOpValidationFunctionReverted(ModuleEntity moduleFunction, bytes revertReason); // Perform the following call, without capturing any return data. // If the call reverts, the revert message will be directly bubbled up. @@ -153,10 +156,14 @@ library ExecutionLib { ModuleEntity moduleEntity, bytes calldata signatureSegment ) internal returns (uint256 validationData) { + bool success; + address moduleAddress; + uint32 entityId; + assembly ("memory-safe") { // Load the module address and entity Id - let entityId := and(shr(64, moduleEntity), 0xffffffff) - let moduleAddress := shr(96, moduleEntity) + entityId := and(shr(64, moduleEntity), 0xffffffff) + moduleAddress := shr(96, moduleEntity) // Update the buffer with the entity Id mstore(add(buffer, 0x24), entityId) @@ -196,35 +203,51 @@ library ExecutionLib { actualCallLength := add(actualCallLength, and(add(signatureSegment.length, 0x1f), not(0x1f))) // Perform the call, reverting on failure or insufficient return data. - switch and( - gt(returndatasize(), 0x1f), - call( - // If gas is the leftmost item before the call, it *should* be placed immediately before the - // call opcode and be allowed in validation. - gas(), - moduleAddress, - /*value*/ - 0, - /*argOffset*/ - add(buffer, 0x20), // jump over 32 bytes for length - /*argSize*/ - actualCallLength, - /*retOffset*/ - 0, - /*retSize*/ - 0x20 + + success := + and( + gt(returndatasize(), 0x1f), + call( + // If gas is the leftmost item before the call, it *should* be placed immediately before the + // call opcode and be allowed in validation. + gas(), + moduleAddress, + /*value*/ + 0, + /*argOffset*/ + add(buffer, 0x20), // jump over 32 bytes for length + /*argSize*/ + actualCallLength, + /*retOffset*/ + 0, + /*retSize*/ + 0x20 + ) ) - ) - case 0 { - // Bubble up the revert if the call reverts. - let m := mload(0x40) - returndatacopy(m, 0, returndatasize()) - revert(m, returndatasize()) - } - default { - // Otherwise, we return the first word of the return data as the validation data + } + + if (success) { + assembly ("memory-safe") { + // If the call was successful, we return the first word of the return data as the validation data. validationData := mload(0) } + } else { + // Revert with the appropriate error type for the selector used. + + uint32 selectorUsed; + uint32 errorSelector; + + assembly ("memory-safe") { + selectorUsed := and(mload(add(buffer, 0x4)), 0xffffffff) + } + + if (selectorUsed == uint32(IValidationHookModule.preUserOpValidationHook.selector)) { + errorSelector = uint32(PreUserOpValidationHookReverted.selector); + } else { + errorSelector = uint32(UserOpValidationFunctionReverted.selector); + } + + _revertModuleFunction(errorSelector, moduleAddress, entityId); } } @@ -331,7 +354,7 @@ library ExecutionLib { } if (!success) { - revert PreRuntimeValidationHookFailed(moduleAddress, entityId, collectReturnData()); + _revertModuleFunction(uint32(PreRuntimeValidationHookReverted.selector), moduleAddress, entityId); } } @@ -432,7 +455,7 @@ library ExecutionLib { } if (!success) { - revert RuntimeValidationFunctionReverted(moduleAddress, entityId, collectReturnData()); + _revertModuleFunction(uint32(RuntimeValidationFunctionReverted.selector), moduleAddress, entityId); } } @@ -615,7 +638,7 @@ library ExecutionLib { } if (!success) { - revert PreExecHookReverted(moduleAddress, entityId, collectReturnData()); + _revertModuleFunction(uint32(PreExecHookReverted.selector), moduleAddress, entityId); } } @@ -808,7 +831,7 @@ library ExecutionLib { } if (!success) { - revert PostExecHookReverted(moduleAddress, entityId, collectReturnData()); + _revertModuleFunction(uint32(PostExecHookReverted.selector), moduleAddress, entityId); } } } @@ -844,10 +867,14 @@ library ExecutionLib { HookConfig hookEntity, bytes calldata signatureSegment ) internal view { + bool success; + address moduleAddress; + uint32 entityId; + assembly ("memory-safe") { // Load the module address and entity id - let entityId := and(shr(64, hookEntity), 0xffffffff) - let moduleAddress := shr(96, hookEntity) + entityId := and(shr(64, hookEntity), 0xffffffff) + moduleAddress := shr(96, hookEntity) // Update the buffer with the entity Id mstore(add(buffer, 0x44), entityId) @@ -873,7 +900,7 @@ library ExecutionLib { let actualCallLength := add(0xa4, and(add(signatureSegment.length, 0x1f), not(0x1f))) // Perform the call - let success := + success := staticcall( gas(), moduleAddress, @@ -886,13 +913,10 @@ library ExecutionLib { /*retSize*/ 0x20 ) + } - if iszero(success) { - // Bubble up the revert if the call reverts. - let m := mload(0x40) - returndatacopy(m, 0, returndatasize()) - revert(m, returndatasize()) - } + if (!success) { + _revertModuleFunction(uint32(PreSignatureValidationHookReverted.selector), moduleAddress, entityId); } } @@ -901,10 +925,14 @@ library ExecutionLib { ModuleEntity validationFunction, bytes calldata signatureSegment ) internal view returns (bytes4 result) { + bool success; + address moduleAddress; + uint32 entityId; + assembly ("memory-safe") { // Load the module address and entity id - let entityId := and(shr(64, validationFunction), 0xffffffff) - let moduleAddress := shr(96, validationFunction) + entityId := and(shr(64, validationFunction), 0xffffffff) + moduleAddress := shr(96, validationFunction) // Store the account in the `account` field. mstore(add(buffer, 0x24), address()) @@ -937,34 +965,34 @@ library ExecutionLib { let actualCallLength := add(0xc4, and(add(signatureSegment.length, 0x1f), not(0x1f))) // Perform the call - switch and( - gt(returndatasize(), 0x1f), - staticcall( - gas(), - moduleAddress, - /*argOffset*/ - add(buffer, 0x20), // jump over 32 bytes for length, and another 32 bytes for the account - /*argSize*/ - actualCallLength, - /*retOffset*/ - 0, - /*retSize*/ - 0x20 + success := + and( + gt(returndatasize(), 0x1f), + staticcall( + gas(), + moduleAddress, + /*argOffset*/ + add(buffer, 0x20), // jump over 32 bytes for length, and another 32 bytes for the account + /*argSize*/ + actualCallLength, + /*retOffset*/ + 0, + /*retSize*/ + 0x20 + ) ) - ) - case 0 { - // Bubble up the revert if the call reverts. - let m := mload(0x40) - returndatacopy(m, 0, returndatasize()) - revert(m, returndatasize()) - } - default { + } + + if (success) { + assembly ("memory-safe") { // Otherwise, we return the first word of the return data as the signature validation result result := mload(0) // If any of the lower 28 bytes are nonzero, it would be an abi decoding failure. if shl(32, result) { revert(0, 0) } } + } else { + _revertModuleFunction(uint32(SignatureValidationFunctionReverted.selector), moduleAddress, entityId); } } @@ -1029,6 +1057,37 @@ library ExecutionLib { return workingMemPtr; } + function _revertModuleFunction(uint32 errorSelector, address moduleAddress, uint32 entityId) private pure { + // All of the module function reverts have the same parameter layout: + // - module address + // - entity Id + // - revert data + + assembly ("memory-safe") { + let m := mload(0x40) + // Write in order of entityId -> address -> selector, to avoid masking or shifts. + mstore(add(m, 0x18), entityId) + mstore(add(m, 0x14), moduleAddress) + mstore(m, errorSelector) + mstore(add(m, 0x40), 0x40) // fixed offset for the revert data + mstore(add(m, 0x60), returndatasize()) + + if returndatasize() { + // Store a zero in the last word of the revert data, to do strict ABI-encoding for the error. + let roundedDownDataLength := and(returndatasize(), not(0x1f)) + mstore(add(m, add(0x80, roundedDownDataLength)), 0) + returndatacopy(add(m, 0x80), 0, returndatasize()) + } + + let roundedUpDataLength := and(add(returndatasize(), 0x1f), not(0x1f)) + + // 4 bytes for the selector, and 0x60 for the 3 words of fixed-size data. + let totalRevertDataLength := add(0x64, roundedUpDataLength) + + revert(add(m, 0x1c), totalRevertDataLength) + } + } + function _prepareRuntimeCallBufferPreValidationHooks(RTCallBuffer buffer) private pure { uint32 selector = uint32(IValidationHookModule.preRuntimeValidationHook.selector); assembly ("memory-safe") { diff --git a/test/account/GlobalValidationTest.t.sol b/test/account/GlobalValidationTest.t.sol index 0f34318d8..847a9e81f 100644 --- a/test/account/GlobalValidationTest.t.sol +++ b/test/account/GlobalValidationTest.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.26; import {ModuleEntityLib} from "@erc6900/reference-implementation/libraries/ModuleEntityLib.sol"; +import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; @@ -75,4 +76,42 @@ contract GlobalValidationTest is AccountTestBase { assertEq(ethRecipient.balance, 2 wei); } + + function test_globalValidation_failsOnSelectorApplicability() public withSMATest { + PackedUserOperation memory userOp = PackedUserOperation({ + sender: address(account2), + nonce: 0, + initCode: abi.encodePacked( + address(factory), abi.encodeCall(factory.createAccount, (owner2, 0, TEST_DEFAULT_VALIDATION_ENTITY_ID)) + ), + callData: abi.encodeCall(ModularAccountBase.execute, (ethRecipient, 1 wei, "")), + accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), + preVerificationGas: 0, + gasFees: _encodeGas(1, 1), + paymasterAndData: "", + signature: "" + }); + + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, userOpHash.toEthSignedMessageHash()); + // Use the wrong checking mode - SELECTOR_ASSOCIATED_VALIDATION + userOp.signature = _encodeSignature( + _signerValidation, SELECTOR_ASSOCIATED_VALIDATION, abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v) + ); + + PackedUserOperation[] memory userOps = new PackedUserOperation[](1); + userOps[0] = userOp; + + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector( + ModularAccountBase.ValidationFunctionMissing.selector, ModularAccountBase.execute.selector + ) + ) + ); + entryPoint.handleOps(userOps, beneficiary); + } } diff --git a/test/account/ModularAccount.t.sol b/test/account/ModularAccount.t.sol index ca082ba8d..98699f2f1 100644 --- a/test/account/ModularAccount.t.sol +++ b/test/account/ModularAccount.t.sol @@ -11,6 +11,7 @@ import {ValidationConfigLib} from "@erc6900/reference-implementation/libraries/V import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; +import {IERC165} from "@openzeppelin/contracts/interfaces/IERC165.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; @@ -22,6 +23,8 @@ import {SemiModularAccountBytecode} from "../../src/account/SemiModularAccountBy import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol"; import {Counter} from "../mocks/Counter.sol"; +import {MockInterface} from "../mocks/MockInterface.sol"; +import {MockRevertingConstructor} from "../mocks/MockRevertingConstructor.sol"; import {ComprehensiveModule} from "../mocks/modules/ComprehensiveModule.sol"; import {MockExecutionInstallationModule} from "../mocks/modules/MockExecutionInstallationModule.sol"; import {MockModule} from "../mocks/modules/MockModule.sol"; @@ -472,7 +475,8 @@ contract ModularAccountTest is AccountTestBase { vm.expectRevert( abi.encodeWithSelector( - ModularAccountBase.SignatureValidationInvalid.selector, singleSignerValidationModule, newEntityId + ModularAccountBase.SignatureValidationInvalid.selector, + ModuleEntityLib.pack(address(singleSignerValidationModule), newEntityId) ) ); IERC1271(address(account1)).isValidSignature(message, signature); @@ -519,7 +523,8 @@ contract ModularAccountTest is AccountTestBase { 0, "AA23 reverted", abi.encodeWithSelector( - ModularAccountBase.UserOpValidationInvalid.selector, singleSignerValidationModule, newEntityId + ModularAccountBase.UserOpValidationInvalid.selector, + ModuleEntityLib.pack(address(singleSignerValidationModule), newEntityId) ) ) ); @@ -547,6 +552,11 @@ contract ModularAccountTest is AccountTestBase { assertEq(returnedAddr, expectedAddr); assertEq(address(ModularAccount(payable(expectedAddr)).entryPoint()), address(entryPoint)); + + // Assert a reverting constructor causes a `CreateFailed` error + vm.prank(address(entryPoint)); + vm.expectRevert(ModularAccountBase.CreateFailed.selector); + account1.performCreate(0, abi.encodePacked(type(MockRevertingConstructor).creationCode)); } function test_performCreate2() public withSMATest { @@ -581,6 +591,74 @@ contract ModularAccountTest is AccountTestBase { account1.executeUserOp(userOp, bytes32(0)); } + function test_revertOnNoInstalledModuleExecFunction() public withSMATest { + bytes4 nonInstalledSelector = MockInterface.doSomething.selector; + + vm.prank(beneficiary); + vm.expectRevert( + abi.encodeWithSelector(ModularAccountBase.UnrecognizedFunction.selector, nonInstalledSelector) + ); + MockInterface(address(account1)).doSomething(); + } + + function test_modularAccountBase_supportsInterface() public withSMATest { + // Assert that the invalid interface ID is not supported + assertFalse(account1.supportsInterface(0xffffffff)); + + // Assert that the ERC165 interface ID is supported + assertTrue(account1.supportsInterface(type(IERC165).interfaceId)); + + // Assert that the interfaceId for a new interface starts out unsupported, but becomes supported after + // install. + assertFalse(account1.supportsInterface(type(MockInterface).interfaceId)); + + // Install the interface id. + ExecutionManifest memory manifest; + manifest.interfaceIds = new bytes4[](1); + manifest.interfaceIds[0] = type(MockInterface).interfaceId; + + vm.prank(address(entryPoint)); + account1.installExecution(makeAddr("mockModule"), manifest, ""); + + assertTrue(account1.supportsInterface(type(MockInterface).interfaceId)); + } + + function test_validationRevertsOnShortCalldata() public withSMATest { + PackedUserOperation memory userOp = PackedUserOperation({ + sender: address(account1), + nonce: 0, + initCode: "", + callData: abi.encodePacked(bytes3(ModularAccountBase.execute.selector)), // Short calldata + accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), + preVerificationGas: 0, + gasFees: _encodeGas(1, 1), + paymasterAndData: "", + signature: "" + }); + + // Generate signature + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); + userOp.signature = + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); + + PackedUserOperation[] memory userOps = new PackedUserOperation[](1); + userOps[0] = userOp; + + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector( + ModularAccountBase.UnrecognizedFunction.selector, + bytes4(bytes3(ModularAccountBase.execute.selector)) + ) + ) + ); + entryPoint.handleOps(userOps, beneficiary); + } + // Internal Functions function _printStorageReadsAndWrites(address addr) internal { diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol index b76dc69e7..e548aa920 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -10,6 +10,7 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol"; +import {ExecutionLib} from "../../src/libraries/ExecutionLib.sol"; import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; @@ -59,9 +60,8 @@ contract MultiValidationTest is AccountTestBase { vm.prank(owner1); vm.expectRevert( abi.encodeWithSelector( - ModularAccountBase.RuntimeValidationFunctionReverted.selector, - address(validator2), - TEST_DEFAULT_VALIDATION_ENTITY_ID, + ExecutionLib.RuntimeValidationFunctionReverted.selector, + ModuleEntityLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID), abi.encodeWithSignature("NotAuthorized()") ) ); diff --git a/test/account/PerHookData.t.sol b/test/account/PerHookData.t.sol index 01b9e2dc4..f7b715fc5 100644 --- a/test/account/PerHookData.t.sol +++ b/test/account/PerHookData.t.sol @@ -11,6 +11,7 @@ import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interface import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol"; +import {ExecutionLib} from "../../src/libraries/ExecutionLib.sol"; import {Counter} from "../mocks/Counter.sol"; import {MockAccessControlHookModule} from "../mocks/modules/MockAccessControlHookModule.sol"; @@ -83,7 +84,11 @@ contract PerHookDataTest is CustomValidationTestBase { IEntryPoint.FailedOpWithRevert.selector, 0, "AA23 reverted", - abi.encodeWithSignature("Error(string)", "Proof doesn't match target") + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(_accessControlHookModule), _PRE_HOOK_ENTITY_ID_1), + abi.encodeWithSignature("Error(string)", "Proof doesn't match target") + ) ) ); entryPoint.handleOps(userOps, beneficiary); @@ -103,7 +108,11 @@ contract PerHookDataTest is CustomValidationTestBase { IEntryPoint.FailedOpWithRevert.selector, 0, "AA23 reverted", - abi.encodeWithSignature("Error(string)", "Proof doesn't match target") + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(_accessControlHookModule), _PRE_HOOK_ENTITY_ID_1), + abi.encodeWithSignature("Error(string)", "Proof doesn't match target") + ) ) ); entryPoint.handleOps(userOps, beneficiary); @@ -222,7 +231,11 @@ contract PerHookDataTest is CustomValidationTestBase { IEntryPoint.FailedOpWithRevert.selector, 0, "AA23 reverted", - abi.encodeWithSignature("Error(string)", "Target not allowed") + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(_accessControlHookModule), _PRE_HOOK_ENTITY_ID_1), + abi.encodeWithSignature("Error(string)", "Target not allowed") + ) ) ); entryPoint.handleOps(userOps, beneficiary); @@ -280,9 +293,8 @@ contract PerHookDataTest is CustomValidationTestBase { vm.prank(owner1); vm.expectRevert( abi.encodeWithSelector( - ModularAccountBase.PreRuntimeValidationHookFailed.selector, - _accessControlHookModule, - _PRE_HOOK_ENTITY_ID_1, + ExecutionLib.PreRuntimeValidationHookReverted.selector, + ModuleEntityLib.pack(address(_accessControlHookModule), _PRE_HOOK_ENTITY_ID_1), abi.encodeWithSignature("Error(string)", "Proof doesn't match target") ) ); @@ -298,9 +310,8 @@ contract PerHookDataTest is CustomValidationTestBase { vm.prank(owner1); vm.expectRevert( abi.encodeWithSelector( - ModularAccountBase.PreRuntimeValidationHookFailed.selector, - _accessControlHookModule, - _PRE_HOOK_ENTITY_ID_1, + ExecutionLib.PreRuntimeValidationHookReverted.selector, + ModuleEntityLib.pack(address(_accessControlHookModule), _PRE_HOOK_ENTITY_ID_1), abi.encodeWithSignature("Error(string)", "Proof doesn't match target") ) ); @@ -373,9 +384,8 @@ contract PerHookDataTest is CustomValidationTestBase { vm.prank(owner1); vm.expectRevert( abi.encodeWithSelector( - ModularAccountBase.PreRuntimeValidationHookFailed.selector, - _accessControlHookModule, - _PRE_HOOK_ENTITY_ID_1, + ExecutionLib.PreRuntimeValidationHookReverted.selector, + ModuleEntityLib.pack(address(_accessControlHookModule), _PRE_HOOK_ENTITY_ID_1), abi.encodeWithSignature("Error(string)", "Target not allowed") ) ); @@ -433,7 +443,13 @@ contract PerHookDataTest is CustomValidationTestBase { validationData: abi.encodePacked(address(0x1234123412341234123412341234123412341234)) }); - vm.expectRevert("Preimage not provided"); + vm.expectRevert( + abi.encodeWithSelector( + ExecutionLib.PreSignatureValidationHookReverted.selector, + ModuleEntityLib.pack(address(_accessControlHookModule), _PRE_HOOK_ENTITY_ID_1), + abi.encodeWithSignature("Error(string)", "Preimage not provided") + ) + ); account1.isValidSignature( messageHash, _encode1271Signature( @@ -449,7 +465,13 @@ contract PerHookDataTest is CustomValidationTestBase { (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, messageHash); - vm.expectRevert("Preimage not provided"); + vm.expectRevert( + abi.encodeWithSelector( + ExecutionLib.PreSignatureValidationHookReverted.selector, + ModuleEntityLib.pack(address(_accessControlHookModule), _PRE_HOOK_ENTITY_ID_1), + abi.encodeWithSignature("Error(string)", "Preimage not provided") + ) + ); account1.isValidSignature(messageHash, _encode1271Signature(_signerValidation, abi.encodePacked(r, s, v))); } diff --git a/test/account/UOCallBuffer.t.sol b/test/account/UOCallBuffer.t.sol index 6ac7deb7d..f5dee3412 100644 --- a/test/account/UOCallBuffer.t.sol +++ b/test/account/UOCallBuffer.t.sol @@ -4,12 +4,12 @@ pragma solidity ^0.8.26; import {ExecutionManifest} from "@erc6900/reference-implementation/interfaces/IExecutionModule.sol"; import {IValidationHookModule} from "@erc6900/reference-implementation/interfaces/IValidationHookModule.sol"; import {IValidationModule} from "@erc6900/reference-implementation/interfaces/IValidationModule.sol"; - import {HookConfigLib} from "@erc6900/reference-implementation/libraries/HookConfigLib.sol"; import {ModuleEntity, ModuleEntityLib} from "@erc6900/reference-implementation/libraries/ModuleEntityLib.sol"; import {ValidationConfigLib} from "@erc6900/reference-implementation/libraries/ValidationConfigLib.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {ExecutionLib} from "../../src/libraries/ExecutionLib.sol"; import {MockModule} from "../mocks/modules/MockModule.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; @@ -227,7 +227,13 @@ contract UOCallBufferTest is AccountTestBase { ); vm.prank(address(entryPoint)); - vm.expectRevert(bytes(hex"abcdabcd")); + vm.expectRevert( + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(validationHooks[0]), 0), + bytes(hex"abcdabcd") + ) + ); account1.validateUserOp(userOp, userOpHash, 1 wei); vm.clearMockedCalls(); diff --git a/test/account/ValidationAssocHooks.t.sol b/test/account/ValidationAssocHooks.t.sol index 41d2995b6..dae37a37a 100644 --- a/test/account/ValidationAssocHooks.t.sol +++ b/test/account/ValidationAssocHooks.t.sol @@ -4,7 +4,9 @@ pragma solidity ^0.8.26; import {ExecutionManifest} from "@erc6900/reference-implementation/interfaces/IExecutionModule.sol"; import {HookConfigLib} from "@erc6900/reference-implementation/libraries/HookConfigLib.sol"; import {ValidationConfigLib} from "@erc6900/reference-implementation/libraries/ValidationConfigLib.sol"; +import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; +import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol"; import {ModuleManagerInternals} from "../../src/account/ModuleManagerInternals.sol"; import {MockModule} from "../mocks/modules/MockModule.sol"; @@ -23,7 +25,7 @@ contract ValidationAssocHooksTest is AccountTestBase { } } - function test_validationAssocHooks_maxValidationHooks() public { + function test_validationAssocHooks_maxValidationHooks() public withSMATest { // Attempt to install 257 validation hooks, expect a revert. bytes[] memory hookInstalls = new bytes[](257); @@ -48,7 +50,7 @@ contract ValidationAssocHooksTest is AccountTestBase { ); } - function test_validationAssocHooks_maxExecHooks() public { + function test_validationAssocHooks_maxExecHooks() public withSMATest { // Attempt to install 257 exec hooks, expect a revert. bytes[] memory hookInstalls = new bytes[](257); @@ -77,4 +79,40 @@ contract ValidationAssocHooksTest is AccountTestBase { hookInstalls ); } + + function test_revertOnMissingExecuteUserOp() public withSMATest { + // install a validation-association execution hook, and expect a revert unless called via `executeUserOp`. + + ExecutionManifest memory m; // empty manifest + + hooks.push(new MockModule(m)); + + bytes[] memory hookInstalls = new bytes[](1); + hookInstalls[0] = abi.encodePacked( + HookConfigLib.packExecHook({_module: address(hooks[0]), _entityId: 0, _hasPre: false, _hasPost: false}) + ); + + account1.installValidation( + ValidationConfigLib.pack({ + _validationFunction: _signerValidation, + _isGlobal: true, + _isSignatureValidation: true, + _isUserOpValidation: true + }), + new bytes4[](0), + "", + hookInstalls + ); + + _runExecUserOp( + makeAddr("target"), + "", + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector(ModularAccountBase.RequireUserOperationContext.selector) + ) + ); + } } diff --git a/test/account/ValidationIntersection.t.sol b/test/account/ValidationIntersection.t.sol index 33f706e44..8521672a2 100644 --- a/test/account/ValidationIntersection.t.sol +++ b/test/account/ValidationIntersection.t.sol @@ -203,8 +203,9 @@ contract ValidationIntersectionTest is AccountTestBase { vm.expectRevert( abi.encodeWithSelector( ModularAccountBase.UnexpectedAggregator.selector, - address(oneHookModule), - MockBaseUserOpValidationModule.EntityId.PRE_VALIDATION_HOOK_1, + ModuleEntityLib.pack( + address(oneHookModule), uint32(MockBaseUserOpValidationModule.EntityId.PRE_VALIDATION_HOOK_1) + ), badAuthorizer ) ); diff --git a/test/mocks/MockInterface.sol b/test/mocks/MockInterface.sol new file mode 100644 index 000000000..3df7beff1 --- /dev/null +++ b/test/mocks/MockInterface.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.26; + +interface MockInterface { + function doSomething() external; +} diff --git a/test/mocks/MockRevertingConstructor.sol b/test/mocks/MockRevertingConstructor.sol new file mode 100644 index 000000000..99b8d1ae0 --- /dev/null +++ b/test/mocks/MockRevertingConstructor.sol @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.26; + +contract MockRevertingConstructor { + constructor() { + revert("Constructor reverts"); + } +} diff --git a/test/modules/AllowlistModule.t.sol b/test/modules/AllowlistModule.t.sol index de9842aba..5afee202e 100644 --- a/test/modules/AllowlistModule.t.sol +++ b/test/modules/AllowlistModule.t.sol @@ -10,6 +10,8 @@ import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interface import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol"; + +import {ExecutionLib} from "../../src/libraries/ExecutionLib.sol"; import {BaseModule} from "../../src/modules/BaseModule.sol"; import {AllowlistModule} from "../../src/modules/permissions/AllowlistModule.sol"; @@ -314,7 +316,11 @@ contract AllowlistModuleTest is CustomValidationTestBase { IEntryPoint.FailedOpWithRevert.selector, 0, "AA23 reverted", - abi.encodeWithSelector(BaseModule.UnexpectedDataPassed.selector) + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(allowlistModule), HOOK_ENTITY_ID), + abi.encodeWithSelector(BaseModule.UnexpectedDataPassed.selector) + ) ) ); entryPoint.handleOps(userOps, beneficiary); @@ -411,7 +417,11 @@ contract AllowlistModuleTest is CustomValidationTestBase { IEntryPoint.FailedOpWithRevert.selector, 0, "AA23 reverted", - abi.encodeWithSelector(AllowlistModule.SelectorNotAllowed.selector) + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(allowlistModule), HOOK_ENTITY_ID), + abi.encodeWithSelector(AllowlistModule.SelectorNotAllowed.selector) + ) ); } } else { @@ -419,7 +429,11 @@ contract AllowlistModuleTest is CustomValidationTestBase { IEntryPoint.FailedOpWithRevert.selector, 0, "AA23 reverted", - abi.encodeWithSelector(AllowlistModule.AddressNotAllowed.selector) + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(allowlistModule), HOOK_ENTITY_ID), + abi.encodeWithSelector(AllowlistModule.AddressNotAllowed.selector) + ) ); } } @@ -441,17 +455,15 @@ contract AllowlistModuleTest is CustomValidationTestBase { ) ) { return abi.encodeWithSelector( - ModularAccountBase.PreRuntimeValidationHookFailed.selector, - address(allowlistModule), - HOOK_ENTITY_ID, + ExecutionLib.PreRuntimeValidationHookReverted.selector, + ModuleEntityLib.pack(address(allowlistModule), HOOK_ENTITY_ID), abi.encodeWithSelector(AllowlistModule.SelectorNotAllowed.selector) ); } } else { return abi.encodeWithSelector( - ModularAccountBase.PreRuntimeValidationHookFailed.selector, - address(allowlistModule), - HOOK_ENTITY_ID, + ExecutionLib.PreRuntimeValidationHookReverted.selector, + ModuleEntityLib.pack(address(allowlistModule), HOOK_ENTITY_ID), abi.encodeWithSelector(AllowlistModule.AddressNotAllowed.selector) ); } diff --git a/test/modules/NativeTokenLimitModule.t.sol b/test/modules/NativeTokenLimitModule.t.sol index 4260d672a..16b375e70 100644 --- a/test/modules/NativeTokenLimitModule.t.sol +++ b/test/modules/NativeTokenLimitModule.t.sol @@ -13,6 +13,7 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol"; +import {ExecutionLib} from "../../src/libraries/ExecutionLib.sol"; import {BaseModule} from "../../src/modules/BaseModule.sol"; import {NativeTokenLimitModule} from "../../src/modules/permissions/NativeTokenLimitModule.sol"; @@ -106,7 +107,13 @@ contract NativeTokenLimitModuleTest is AccountTestBase { assertEq(result, expected); // uses 200k + 1 wei of gas - vm.expectRevert(NativeTokenLimitModule.ExceededNativeTokenLimit.selector); + vm.expectRevert( + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(module), entityId), + abi.encodeWithSelector(NativeTokenLimitModule.ExceededNativeTokenLimit.selector) + ) + ); result = account1.validateUserOp(_getPackedUO(100_000, 100_000, 1, 1, _getExecuteWithValue(0)), bytes32(0), 0); @@ -123,13 +130,10 @@ contract NativeTokenLimitModuleTest is AccountTestBase { // uses 5e + 1wei of native tokens vm.expectRevert( - abi.encodePacked( - ModularAccountBase.PreExecHookReverted.selector, - abi.encode( - address(module), - uint32(0), - abi.encodePacked(NativeTokenLimitModule.ExceededNativeTokenLimit.selector) - ) + abi.encodeWithSelector( + ExecutionLib.PreExecHookReverted.selector, + ModuleEntityLib.pack(address(module), uint32(0)), + abi.encodePacked(NativeTokenLimitModule.ExceededNativeTokenLimit.selector) ) ); account1.executeUserOp(_getPackedUO(0, 0, 0, 0, _getExecuteWithValue(5 ether + 1)), bytes32(0)); @@ -285,7 +289,11 @@ contract NativeTokenLimitModuleTest is AccountTestBase { IEntryPoint.FailedOpWithRevert.selector, 0, "AA23 reverted", - abi.encodeWithSelector(BaseModule.UnexpectedDataPassed.selector) + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(module), entityId), + abi.encodeWithSelector(BaseModule.UnexpectedDataPassed.selector) + ) ) ); entryPoint.handleOps(uos, beneficiary); diff --git a/test/modules/PaymasterGuardModule.t.sol b/test/modules/PaymasterGuardModule.t.sol index a4b76af09..4a2f8c018 100644 --- a/test/modules/PaymasterGuardModule.t.sol +++ b/test/modules/PaymasterGuardModule.t.sol @@ -10,6 +10,7 @@ import {ModuleEntityLib} from "@erc6900/reference-implementation/libraries/Modul import {ValidationConfigLib} from "@erc6900/reference-implementation/libraries/ValidationConfigLib.sol"; import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol"; +import {ExecutionLib} from "../../src/libraries/ExecutionLib.sol"; import {BaseModule} from "../../src/modules/BaseModule.sol"; import {PaymasterGuardModule} from "../../src/modules/permissions/PaymasterGuardModule.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; @@ -178,7 +179,13 @@ contract PaymasterGuardModuleTest is AccountTestBase { abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v) ); - vm.expectRevert(abi.encodeWithSelector(PaymasterGuardModule.BadPaymasterSpecified.selector)); + vm.expectRevert( + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(module), ENTITY_ID), + abi.encodeWithSelector(PaymasterGuardModule.BadPaymasterSpecified.selector) + ) + ); vm.prank(address(entryPoint)); account1.validateUserOp(userOp, userOpHash, 0); } diff --git a/test/modules/TimeRangeModule.t.sol b/test/modules/TimeRangeModule.t.sol index 539d16e55..d17ddb946 100644 --- a/test/modules/TimeRangeModule.t.sol +++ b/test/modules/TimeRangeModule.t.sol @@ -12,6 +12,7 @@ import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interface import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol"; +import {ExecutionLib} from "../../src/libraries/ExecutionLib.sol"; import {BaseModule} from "../../src/modules/BaseModule.sol"; import {TimeRangeModule} from "../../src/modules/permissions/TimeRangeModule.sol"; @@ -168,9 +169,8 @@ contract TimeRangeModuleTest is CustomValidationTestBase { vm.expectRevert( abi.encodeWithSelector( - ModularAccountBase.PreRuntimeValidationHookFailed.selector, - timeRangeModule, - HOOK_ENTITY_ID, + ExecutionLib.PreRuntimeValidationHookReverted.selector, + ModuleEntityLib.pack(address(timeRangeModule), HOOK_ENTITY_ID), abi.encodeWithSelector(TimeRangeModule.TimeRangeNotValid.selector) ) ); @@ -209,9 +209,8 @@ contract TimeRangeModuleTest is CustomValidationTestBase { vm.expectRevert( abi.encodeWithSelector( - ModularAccountBase.PreRuntimeValidationHookFailed.selector, - timeRangeModule, - HOOK_ENTITY_ID, + ExecutionLib.PreRuntimeValidationHookReverted.selector, + ModuleEntityLib.pack(address(timeRangeModule), HOOK_ENTITY_ID), abi.encodeWithSelector(TimeRangeModule.TimeRangeNotValid.selector) ) ); @@ -249,7 +248,13 @@ contract TimeRangeModuleTest is CustomValidationTestBase { userOp.signature = _encodeSignature(_signerValidation, GLOBAL_VALIDATION, preValidationHookData, ""); vm.prank(address(entryPoint)); - vm.expectRevert(abi.encodeWithSelector(BaseModule.UnexpectedDataPassed.selector)); + vm.expectRevert( + abi.encodeWithSelector( + ExecutionLib.PreUserOpValidationHookReverted.selector, + ModuleEntityLib.pack(address(timeRangeModule), HOOK_ENTITY_ID), + abi.encodeWithSelector(BaseModule.UnexpectedDataPassed.selector) + ) + ); account1.validateUserOp(userOp, userOpHash, 0); }