From 74bd94814a81d88e289f431d5aee1a93df3f078c Mon Sep 17 00:00:00 2001 From: Adam Egyed <5456061+adamegyed@users.noreply.github.com> Date: Sun, 24 Nov 2024 16:09:38 -0800 Subject: [PATCH] fix: [CL ALCHEMY-004] enforce unique deferred action nonce per UO validation (#299) --- gas-snapshots/ModularAccount.json | 28 +-- gas-snapshots/SemiModularAccount.json | 26 +- gas/modular-account/ModularAccount.gas.t.sol | 7 +- .../SemiModularAccount.gas.t.sol | 7 +- src/account/AccountStorage.sol | 2 - src/account/ModularAccountBase.sol | 229 ++++++++---------- src/account/ModularAccountView.sol | 1 - src/account/SemiModularAccountBase.sol | 53 +++- src/helpers/Constants.sol | 3 + src/helpers/NativeFunctionDelegate.sol | 1 - src/interfaces/IModularAccountBase.sol | 4 - src/libraries/ValidationLocatorLib.sol | 50 +++- test/account/DeferredAction.t.sol | 50 +--- test/account/DeferredValidation.t.sol | 106 ++------ test/account/PerHookData.t.sol | 7 +- test/account/UpgradeToSma.t.sol | 9 +- test/modules/PaymasterGuardModule.t.sol | 2 +- test/utils/AccountTestBase.sol | 22 +- test/utils/ModuleSignatureUtils.sol | 38 ++- 19 files changed, 272 insertions(+), 373 deletions(-) diff --git a/gas-snapshots/ModularAccount.json b/gas-snapshots/ModularAccount.json index 1dadd657..cf7c08be 100644 --- a/gas-snapshots/ModularAccount.json +++ b/gas-snapshots/ModularAccount.json @@ -1,16 +1,16 @@ { - "Runtime_AccountCreation": "176376", - "Runtime_BatchTransfers": "92817", - "Runtime_Erc20Transfer": "78271", - "Runtime_InstallSessionKey_Case1": "423293", - "Runtime_NativeTransfer": "54420", - "Runtime_UseSessionKey_Case1_Counter": "78542", - "Runtime_UseSessionKey_Case1_Token": "111855", - "UserOp_BatchTransfers": "197788", - "UserOp_Erc20Transfer": "184784", - "UserOp_InstallSessionKey_Case1": "531124", - "UserOp_NativeTransfer": "161029", - "UserOp_UseSessionKey_Case1_Counter": "194521", - "UserOp_UseSessionKey_Case1_Token": "225505", - "UserOp_deferredValidation": "257223" + "Runtime_AccountCreation": "176354", + "Runtime_BatchTransfers": "92886", + "Runtime_Erc20Transfer": "78318", + "Runtime_InstallSessionKey_Case1": "423362", + "Runtime_NativeTransfer": "54467", + "Runtime_UseSessionKey_Case1_Counter": "78531", + "Runtime_UseSessionKey_Case1_Token": "111844", + "UserOp_BatchTransfers": "197706", + "UserOp_Erc20Transfer": "184680", + "UserOp_InstallSessionKey_Case1": "531042", + "UserOp_NativeTransfer": "160925", + "UserOp_UseSessionKey_Case1_Counter": "194415", + "UserOp_UseSessionKey_Case1_Token": "225399", + "UserOp_deferredValidation": "233002" } \ No newline at end of file diff --git a/gas-snapshots/SemiModularAccount.json b/gas-snapshots/SemiModularAccount.json index 3a2243f0..b6675a60 100644 --- a/gas-snapshots/SemiModularAccount.json +++ b/gas-snapshots/SemiModularAccount.json @@ -1,16 +1,16 @@ { "Runtime_AccountCreation": "97767", - "Runtime_BatchTransfers": "89019", - "Runtime_Erc20Transfer": "74521", - "Runtime_InstallSessionKey_Case1": "422024", - "Runtime_NativeTransfer": "50680", - "Runtime_UseSessionKey_Case1_Counter": "78809", - "Runtime_UseSessionKey_Case1_Token": "112122", - "UserOp_BatchTransfers": "193097", - "UserOp_Erc20Transfer": "180176", - "UserOp_InstallSessionKey_Case1": "528787", - "UserOp_NativeTransfer": "156451", - "UserOp_UseSessionKey_Case1_Counter": "194821", - "UserOp_UseSessionKey_Case1_Token": "225805", - "UserOp_deferredValidation": "253169" + "Runtime_BatchTransfers": "88758", + "Runtime_Erc20Transfer": "74238", + "Runtime_InstallSessionKey_Case1": "421513", + "Runtime_NativeTransfer": "50397", + "Runtime_UseSessionKey_Case1_Counter": "78834", + "Runtime_UseSessionKey_Case1_Token": "112147", + "UserOp_BatchTransfers": "192875", + "UserOp_Erc20Transfer": "179932", + "UserOp_InstallSessionKey_Case1": "528315", + "UserOp_NativeTransfer": "156207", + "UserOp_UseSessionKey_Case1_Counter": "194667", + "UserOp_UseSessionKey_Case1_Token": "225651", + "UserOp_deferredValidation": "228953" } \ No newline at end of file diff --git a/gas/modular-account/ModularAccount.gas.t.sol b/gas/modular-account/ModularAccount.gas.t.sol index 490e991d..97861a2c 100644 --- a/gas/modular-account/ModularAccount.gas.t.sol +++ b/gas/modular-account/ModularAccount.gas.t.sol @@ -13,6 +13,7 @@ import {Vm} from "forge-std/Vm.sol"; import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol"; import {AccountFactory} from "../../src/factory/AccountFactory.sol"; +import {ValidationLocatorLib} from "../../src/libraries/ValidationLocatorLib.sol"; import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol"; import {ModularAccountBenchmarkBase} from "./ModularAccountBenchmarkBase.sol"; @@ -270,11 +271,10 @@ contract ModularAccountGasTest is ModularAccountBenchmarkBase("ModularAccount") (newUOValidation, new bytes4[](0), abi.encode(newEntityId, owner2), new bytes[](0)) ); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; bytes32 digest = _getDeferredInstallStruct( - account1, deferredInstallNonce, deferredInstallDeadline, newUOValidation, deferredValidationInstallCall + account1, userOp.nonce, deferredInstallDeadline, deferredValidationInstallCall ); bytes memory deferredValidationSig = _signRawHash( @@ -285,9 +285,8 @@ contract ModularAccountGasTest is ModularAccountBenchmarkBase("ModularAccount") userOp.signature = _encodeDeferredInstallUOSignature( _packDeferredInstallData( - deferredInstallNonce, deferredInstallDeadline, - ValidationConfigLib.pack(signerValidation, true, false, false), + ValidationLocatorLib.packFromModuleEntity(signerValidation, true, false), deferredValidationInstallCall ), deferredValidationSig, diff --git a/gas/modular-account/SemiModularAccount.gas.t.sol b/gas/modular-account/SemiModularAccount.gas.t.sol index 1972acfb..7ffd7f02 100644 --- a/gas/modular-account/SemiModularAccount.gas.t.sol +++ b/gas/modular-account/SemiModularAccount.gas.t.sol @@ -15,6 +15,7 @@ import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol"; import {AccountFactory} from "../../src/factory/AccountFactory.sol"; import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol"; +import {ValidationLocatorLib} from "../../src/libraries/ValidationLocatorLib.sol"; import {ModularAccountBenchmarkBase} from "./ModularAccountBenchmarkBase.sol"; contract ModularAccountGasTest is ModularAccountBenchmarkBase("SemiModularAccount") { @@ -264,20 +265,18 @@ contract ModularAccountGasTest is ModularAccountBenchmarkBase("SemiModularAccoun (newUOValidation, new bytes4[](0), abi.encode(newEntityId, owner2), new bytes[](0)) ); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; bytes32 digest = _getDeferredInstallStruct( - account1, deferredInstallNonce, deferredInstallDeadline, newUOValidation, deferredValidationInstallCall + account1, userOp.nonce, deferredInstallDeadline, deferredValidationInstallCall ); bytes memory deferredValidationSig = _signRawHash(vm, owner1Key, digest); userOp.signature = _encodeDeferredInstallUOSignature( _packDeferredInstallData( - deferredInstallNonce, deferredInstallDeadline, - ValidationConfigLib.pack(signerValidation, true, false, false), + ValidationLocatorLib.packFromModuleEntity(signerValidation, true, false), deferredValidationInstallCall ), deferredValidationSig, diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 51bce5fa..9382dd69 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -61,8 +61,6 @@ struct AccountStorage { mapping(ValidationLookupKey lookupKey => ValidationStorage) validationStorage; // Module-defined ERC-165 interfaces installed on the account. mapping(bytes4 => uint256) supportedIfaces; - // Nonce usage state for deferred actions. - mapping(uint256 => bool) deferredActionNonceUsed; } function getAccountStorage() pure returns (AccountStorage storage _storage) { diff --git a/src/account/ModularAccountBase.sol b/src/account/ModularAccountBase.sol index 26314432..b767822e 100644 --- a/src/account/ModularAccountBase.sol +++ b/src/account/ModularAccountBase.sol @@ -70,8 +70,6 @@ abstract contract ModularAccountBase is using ValidationConfigLib for ValidationConfig; using HookConfigLib for HookConfig; using SparseCalldataSegmentLib for bytes; - // temp, remove after updating validation selection - using ValidationLocatorLib for ModuleEntity; enum ValidationCheckingType { GLOBAL, @@ -83,9 +81,9 @@ abstract contract ModularAccountBase is bytes32 internal constant _DOMAIN_SEPARATOR_TYPEHASH = 0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218; - // keccak256("DeferredAction(uint256 nonce,uint48 deadline,uint168 validationLocator,bytes call)") + // keccak256("DeferredAction(uint256 nonce,uint48 deadline,bytes call)") bytes32 internal constant _DEFERRED_ACTION_TYPEHASH = - 0xa0a7682d80b18373e0ec8549850bc960f46562e16ae4ddee86ab9a3cc3d3a07c; + 0x9b23e06584efc6b65fc854cee55011d89f86485487b6db36aed7d23884711ea3; // As per the EIP-165 spec, no interface should ever match 0xffffffff bytes4 internal constant _INTERFACE_ID_INVALID = 0xffffffff; @@ -96,10 +94,7 @@ abstract contract ModularAccountBase is address internal immutable _EXECUTION_INSTALL_DELEGATE; - event DeferredActionNonceInvalidated(uint256 nonce); - error CreateFailed(); - error DeferredActionNonceInvalid(); error DeferredActionSignatureInvalid(); error RequireUserOperationContext(); error SelfCallRecursionDepthExceeded(); @@ -273,7 +268,7 @@ abstract contract ModularAccountBase is locator.isGlobal() ? ValidationCheckingType.GLOBAL : ValidationCheckingType.SELECTOR ); - RTCallBuffer rtCallBuffer = _doRuntimeValidation(locator, data, authorizationData); + RTCallBuffer rtCallBuffer = _doRuntimeValidation(locator.lookupKey(), data, authorizationData); // If runtime validation passes, run exec hooks associated with the validator HookConfig[] memory validationAssocExecHooks = MemManagementLib.loadExecHooks(_validationStorage); @@ -330,7 +325,7 @@ abstract contract ModularAccountBase is bytes4[] calldata selectors, bytes calldata installData, bytes[] calldata hooks - ) external wrapNativeFunction { + ) external virtual wrapNativeFunction { _installValidation(validationConfig, selectors, installData, hooks); } @@ -344,18 +339,12 @@ abstract contract ModularAccountBase is _uninstallValidation(validationFunction, uninstallData, hookUninstallData); } - /// @inheritdoc IModularAccountBase - function invalidateDeferredValidationInstallNonce(uint256 nonce) external override wrapNativeFunction { - getAccountStorage().deferredActionNonceUsed[nonce] = true; - emit DeferredActionNonceInvalidated(nonce); - } - /// @inheritdoc IERC1271 function isValidSignature(bytes32 hash, bytes calldata signature) external view override returns (bytes4) { (ValidationLocator locator, bytes calldata signatureRemainder) = ValidationLocatorLib.loadFromSignature(signature); - return _isValidSignature(locator, hash, signatureRemainder); + return _isValidSignature(locator.lookupKey(), hash, signatureRemainder); } /// @inheritdoc IERC165 @@ -440,7 +429,7 @@ abstract contract ModularAccountBase is // validation memory. MemSnapshot memSnapshot = MemManagementLib.freezeFMP(); - uint48 deadline = _handleDeferredAction(locator, encodedData, deferredActionSig); + uint48 deadline = _handleDeferredAction(userOp.nonce, encodedData, deferredActionSig); // Restore the free memory pointer. MemManagementLib.restoreFMP(memSnapshot); @@ -467,7 +456,7 @@ abstract contract ModularAccountBase is ) { revert RequireUserOperationContext(); } - uint256 userOpValidationRes = _doUserOpValidation(userOp, userOpHash, locator, userOpSignature); + uint256 userOpValidationRes = _doUserOpValidation(userOp, userOpHash, locator.lookupKey(), userOpSignature); // We only coalesce validations if the validation data from deferred installation is nonzero. if (validationData != 0) { @@ -478,53 +467,52 @@ abstract contract ModularAccountBase is } /// @return The deadline of the deferred action - function _handleDeferredAction(ValidationLocator locator, bytes calldata encodedData, bytes calldata sig) + function _handleDeferredAction(uint256 userOpNonce, bytes calldata encodedData, bytes calldata sig) internal returns (uint48) { - // The deadline, nonce, inner validation, and deferred call selector are all at fixed positions in the - // encodedData. + // The inner validation, deadline, and deferred call bytes are all at fixed positions in the encoded data. + // [:21] = ValidationLocator defActionValidationLocator + // [21:27] = uint48 deadline + // [27:] = bytes deferredCall - // todo: this will need to get encoded differently - ValidationConfig defActionSigValidation = ValidationConfig.wrap(bytes25(encodedData[38:63])); - bool isGlobalSigValidation = defActionSigValidation.isGlobal(); + ValidationLocator defActionValidationLocator = ValidationLocator.wrap(uint168(bytes21(encodedData[:21]))); - ModuleEntity defActionValidationModuleEntity = defActionSigValidation.moduleEntity(); + ValidationStorage storage _validationStorage = + getAccountStorage().validationStorage[defActionValidationLocator.lookupKey()]; // Because this bypasses UO validation hooks, we require that the validation used does not include any // validation hooks. - if ( - getAccountStorage().validationStorage[defActionValidationModuleEntity.moduleEntityToLookupKey()] - .validationHookCount != 0 - ) { + if (_validationStorage.validationHookCount != 0) { revert DeferredValidationHasValidationHooks(); } + uint48 deadline = uint48(bytes6(encodedData[21:27])); + + bytes32 typedDataHash = _computeDeferredActionHash(userOpNonce, deadline, encodedData[27:]); + // Check if the outer validation applies to the function call _checkIfValidationAppliesCallData( - encodedData[63:], - defActionValidationModuleEntity.moduleEntityToLookupKey(), - isGlobalSigValidation ? ValidationCheckingType.GLOBAL : ValidationCheckingType.SELECTOR + encodedData[27:], + defActionValidationLocator.lookupKey(), + defActionValidationLocator.isGlobal() ? ValidationCheckingType.GLOBAL : ValidationCheckingType.SELECTOR ); // Handle the signature validation - uint48 deadline = - _validateDeferredActionSignature(encodedData, sig, locator, defActionValidationModuleEntity); + _validateDeferredActionSignature(typedDataHash, sig, defActionValidationLocator.lookupKey()); // Run the validation associated execution hooks, allocating a call buffer as needed. - HookConfig[] memory validationAssocExecHooks = MemManagementLib.loadExecHooks( - getAccountStorage().validationStorage[defActionValidationModuleEntity.moduleEntityToLookupKey()] - ); + HookConfig[] memory validationAssocExecHooks = MemManagementLib.loadExecHooks(_validationStorage); PHCallBuffer callBuffer; if (validationAssocExecHooks.length > 0) { - callBuffer = ExecutionLib.allocatePreExecHookCallBuffer(encodedData[63:]); + callBuffer = ExecutionLib.allocatePreExecHookCallBuffer(encodedData[27:]); } DensePostHookData postHookData = ExecutionLib.doPreHooks(validationAssocExecHooks, callBuffer); // Perform the deferred action's self call on the account. - ExecutionLib.callBubbleOnRevertTransient(address(this), 0, encodedData[63:]); + ExecutionLib.callBubbleOnRevertTransient(address(this), 0, encodedData[27:]); // Do the cached post hooks ExecutionLib.doCachedPostHooks(postHookData); @@ -536,20 +524,17 @@ abstract contract ModularAccountBase is function _doUserOpValidation( PackedUserOperation calldata userOp, bytes32 userOpHash, - ValidationLocator validationLocator, + ValidationLookupKey validationLookupKey, bytes calldata signature ) internal returns (uint256) { - ValidationStorage storage _validationStorage = - getAccountStorage().validationStorage[validationLocator.lookupKey()]; + ValidationStorage storage _validationStorage = getAccountStorage().validationStorage[validationLookupKey]; // Do preUserOpValidation hooks HookConfig[] memory preUserOpValidationHooks = MemManagementLib.loadValidationHooks(_validationStorage); - ModuleEntity userOpValidationFunction = validationLocator.moduleEntity(_validationStorage); - uint256 validationRes; UOCallBuffer userOpCallBuffer; - if (!_validationIsNative(userOpValidationFunction) || preUserOpValidationHooks.length > 0) { + if (!_validationIsNative(validationLookupKey) || preUserOpValidationHooks.length > 0) { userOpCallBuffer = ExecutionLib.allocateUserOpValidationCallBuffer(userOp, userOpHash); } bytes calldata currentSignatureSlice; @@ -578,9 +563,8 @@ abstract contract ModularAccountBase is { currentSignatureSlice = signature.getFinalSegment(); - uint256 currentValidationRes = _execUserOpValidation( - userOpValidationFunction, userOpHash, currentSignatureSlice, userOpCallBuffer - ); + uint256 currentValidationRes = + _execUserOpValidation(validationLookupKey, userOpHash, currentSignatureSlice, userOpCallBuffer); if (preUserOpValidationHooks.length != 0) { // If we have other validation data we need to coalesce with @@ -594,20 +578,17 @@ abstract contract ModularAccountBase is } function _doRuntimeValidation( - ValidationLocator validationLocator, + ValidationLookupKey validationLookupKey, bytes calldata callData, bytes calldata authorizationData ) internal returns (RTCallBuffer) { - ValidationStorage storage _validationData = - getAccountStorage().validationStorage[validationLocator.lookupKey()]; + ValidationStorage storage _validationData = getAccountStorage().validationStorage[validationLookupKey]; // run all preRuntimeValidation hooks HookConfig[] memory preRuntimeValidationHooks = MemManagementLib.loadValidationHooks(_validationData); - ModuleEntity runtimeValidationFunction = validationLocator.moduleEntity(_validationData); - RTCallBuffer callBuffer; - if (!_validationIsNative(runtimeValidationFunction) || preRuntimeValidationHooks.length > 0) { + if (!_validationIsNative(validationLookupKey) || preRuntimeValidationHooks.length > 0) { callBuffer = ExecutionLib.allocateRuntimeValidationCallBuffer(callData, authorizationData); } @@ -629,7 +610,7 @@ abstract contract ModularAccountBase is authorizationData = authorizationData.getFinalSegment(); - _execRuntimeValidation(runtimeValidationFunction, callBuffer, authorizationData); + _execRuntimeValidation(validationLookupKey, callBuffer, authorizationData); return callBuffer; } @@ -710,14 +691,17 @@ abstract contract ModularAccountBase is } function _execUserOpValidation( - ModuleEntity userOpValidationFunction, - bytes32, + ValidationLookupKey validationLookupKey, + bytes32 hash, bytes calldata signatureSegment, UOCallBuffer callBuffer ) internal virtual returns (uint256) { - AccountStorage storage _storage = getAccountStorage(); + (hash); // unused in ModularAccountBase, but used in SemiModularAccountBase + ValidationStorage storage _validationStorage = getAccountStorage().validationStorage[validationLookupKey]; + + ModuleEntity userOpValidationFunction = validationLookupKey.moduleEntity(_validationStorage); - if (!_storage.validationStorage[userOpValidationFunction.moduleEntityToLookupKey()].isUserOpValidation) { + if (!_validationStorage.isUserOpValidation) { revert UserOpValidationInvalid(userOpValidationFunction); } @@ -727,30 +711,26 @@ abstract contract ModularAccountBase is } function _execRuntimeValidation( - ModuleEntity runtimeValidationFunction, + ValidationLookupKey validationLookupKey, RTCallBuffer callBuffer, bytes calldata authorization ) internal virtual { + ValidationStorage storage _validationData = getAccountStorage().validationStorage[validationLookupKey]; + ModuleEntity runtimeValidationFunction = validationLookupKey.moduleEntity(_validationData); ExecutionLib.invokeRuntimeCallBufferValidation(callBuffer, runtimeValidationFunction, authorization); } - function _validateDeferredActionSignature( - bytes calldata encodedData, - bytes calldata signature, - ValidationLocator locator, - ModuleEntity deferredSigValidationModuleEntity - ) internal returns (uint48) { - uint256 nonce = uint256(bytes32(encodedData[:32])); - uint48 deadline = uint48(bytes6(encodedData[32:38])); - - // Check that the passed nonce isn't already invalidated. - if (getAccountStorage().deferredActionNonceUsed[nonce]) { - revert DeferredActionNonceInvalid(); - } - - // Invalidate the nonce. - getAccountStorage().deferredActionNonceUsed[nonce] = true; - emit DeferredActionNonceInvalidated(nonce); + function _computeDeferredActionHash(uint256 userOpNonce, uint48 deadline, bytes calldata selfCall) + internal + view + returns (bytes32) + { + // Note: + // - A zero deadline translates to "no deadline" + // - The user op nonce also includes the data for: + // - Which validation function to use + // - Whether or not a deferred action is included + // - Whether or not the validation is used as a global validation. // Compute the hash without permanently allocating memory for each step. // The following is equivalent to: @@ -759,78 +739,69 @@ abstract contract ModularAccountBase is // _DEFERRED_ACTION_TYPEHASH, // nonce, // deadline, - // validationFunction, // keccak256(selfCall) // ) // ) - // Note that a zero deadline translates to "no deadline" - - // Fetch the self-call from the encoded data. - bytes calldata selfCall = encodedData[63:]; + // Compute the struct hash, then convert it to a typed data hash. + bytes32 structHash; - // Compute the typed data hash. - bytes32 typedDataHash; - { - bytes32 structHash; + assembly ("memory-safe") { + // Get the hash of the dynamic-length encoded install call + let fmp := mload(0x40) + calldatacopy(fmp, selfCall.offset, selfCall.length) + let selfCallHash := keccak256(fmp, selfCall.length) + + // Compute the struct hash + let ptr := fmp + mstore(ptr, _DEFERRED_ACTION_TYPEHASH) + ptr := add(ptr, 0x20) + mstore(ptr, userOpNonce) + ptr := add(ptr, 0x20) + // Clear the upper bits of the deadline, in case the caller didn't. + mstore(ptr, and(deadline, 0xffffffffffff)) + ptr := add(ptr, 0x20) + mstore(ptr, selfCallHash) + + // Compute the struct hash + structHash := keccak256(fmp, 0x80) + } - assembly ("memory-safe") { - // Get the hash of the dynamic-length encoded install call - let fmp := mload(0x40) - calldatacopy(fmp, selfCall.offset, selfCall.length) - let selfCallHash := keccak256(fmp, selfCall.length) - - // Compute the struct hash - let ptr := fmp - mstore(ptr, _DEFERRED_ACTION_TYPEHASH) - ptr := add(ptr, 0x20) - mstore(ptr, nonce) - ptr := add(ptr, 0x20) - // Clear the upper bits of the deadline, in case the caller didn't. - mstore(ptr, and(deadline, 0xffffffffffff)) - ptr := add(ptr, 0x20) - // Clear the upper bits of the validation locator, in case the caller didn't. - mstore(ptr, and(locator, 0xffffffffffffffffffffffffffffffffffffffffff)) - ptr := add(ptr, 0x20) - mstore(ptr, selfCallHash) - - // Compute the struct hash - structHash := keccak256(fmp, 0xa0) - } + bytes32 typedDataHash = MessageHashUtils.toTypedDataHash(_domainSeparator(), structHash); - typedDataHash = MessageHashUtils.toTypedDataHash(_domainSeparator(), structHash); - } + return typedDataHash; + } + function _validateDeferredActionSignature( + bytes32 defActionTypedDataHash, + bytes calldata signature, + ValidationLookupKey deferredSigValidationLookupKey + ) internal view { // Validate the 1271 signature. SigCallBuffer sigCallBuffer; - if (!_validationIsNative(deferredSigValidationModuleEntity)) { - sigCallBuffer = ExecutionLib.allocateSigCallBuffer(typedDataHash, signature); + if (!_validationIsNative(deferredSigValidationLookupKey)) { + sigCallBuffer = ExecutionLib.allocateSigCallBuffer(defActionTypedDataHash, signature); } if ( - _exec1271Validation(sigCallBuffer, typedDataHash, deferredSigValidationModuleEntity, signature) + _exec1271Validation(sigCallBuffer, defActionTypedDataHash, deferredSigValidationLookupKey, signature) != _1271_MAGIC_VALUE ) { revert DeferredActionSignatureInvalid(); } - - return deadline; } - function _isValidSignature(ValidationLocator validationLocator, bytes32 hash, bytes calldata signature) + function _isValidSignature(ValidationLookupKey validationLookupKey, bytes32 hash, bytes calldata signature) internal view returns (bytes4) { - ValidationStorage storage _validationStorage = - getAccountStorage().validationStorage[validationLocator.lookupKey()]; + ValidationStorage storage _validationStorage = getAccountStorage().validationStorage[validationLookupKey]; HookConfig[] memory preSignatureValidationHooks = MemManagementLib.loadValidationHooks(_validationStorage); - ModuleEntity sigValidation = validationLocator.moduleEntity(_validationStorage); - SigCallBuffer sigCallBuffer; - if (!_validationIsNative(sigValidation) || preSignatureValidationHooks.length > 0) { + if (!_validationIsNative(validationLookupKey) || preSignatureValidationHooks.length > 0) { sigCallBuffer = ExecutionLib.allocateSigCallBuffer(hash, signature); } for (uint256 i = preSignatureValidationHooks.length; i > 0;) { @@ -850,19 +821,21 @@ abstract contract ModularAccountBase is } signature = signature.getFinalSegment(); - return _exec1271Validation(sigCallBuffer, hash, sigValidation, signature); + return _exec1271Validation(sigCallBuffer, hash, validationLookupKey, signature); } function _exec1271Validation( SigCallBuffer buffer, bytes32 hash, - ModuleEntity sigValidation, + ValidationLookupKey validationLookupKey, bytes calldata signatureSegment ) internal view virtual returns (bytes4) { (hash); // unused in ModularAccountBase, but used in SemiModularAccountBase - AccountStorage storage _storage = getAccountStorage(); + ValidationStorage storage _validationStorage = getAccountStorage().validationStorage[validationLookupKey]; - if (!_storage.validationStorage[sigValidation.moduleEntityToLookupKey()].isSignatureValidation) { + ModuleEntity sigValidation = validationLookupKey.moduleEntity(_validationStorage); + + if (!_validationStorage.isSignatureValidation) { revert SignatureValidationInvalid(sigValidation); } @@ -881,8 +854,6 @@ abstract contract ModularAccountBase is ValidationLookupKey validationFunction, ValidationCheckingType checkingType ) internal view { - // address(4).staticcall(abi.encodePacked(validationFunction)); - if (callData.length < 4) { revert UnrecognizedFunction(bytes4(callData)); } @@ -1134,7 +1105,7 @@ abstract contract ModularAccountBase is // A virtual function to detect if a validation function is natively implemented. Used for determining call // buffer allocation. - function _validationIsNative(ModuleEntity) internal pure virtual returns (bool) { + function _validationIsNative(ValidationLookupKey) internal pure virtual returns (bool) { return false; } } diff --git a/src/account/ModularAccountView.sol b/src/account/ModularAccountView.sol index e0e31ab1..e12429f3 100644 --- a/src/account/ModularAccountView.sol +++ b/src/account/ModularAccountView.sol @@ -91,7 +91,6 @@ abstract contract ModularAccountView is IModularAccountView { || selector == IModularAccount.installValidation.selector || selector == IModularAccount.uninstallValidation.selector || selector == UUPSUpgradeable.upgradeToAndCall.selector - || selector == IModularAccountBase.invalidateDeferredValidationInstallNonce.selector || selector == IModularAccountBase.performCreate.selector ) { return true; diff --git a/src/account/SemiModularAccountBase.sol b/src/account/SemiModularAccountBase.sol index 8a28661a..72d71ce5 100644 --- a/src/account/SemiModularAccountBase.sol +++ b/src/account/SemiModularAccountBase.sol @@ -1,14 +1,15 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.26; -import {ModuleEntity} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; +import {ModuleEntity, ValidationConfig} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; import {ModuleEntityLib} from "@erc6900/reference-implementation/libraries/ModuleEntityLib.sol"; +import {ValidationConfigLib} from "@erc6900/reference-implementation/libraries/ValidationConfigLib.sol"; import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; -import {FALLBACK_VALIDATION, FALLBACK_VALIDATION_LOOKUP_KEY} from "../helpers/Constants.sol"; +import {FALLBACK_VALIDATION_ID, FALLBACK_VALIDATION_LOOKUP_KEY} from "../helpers/Constants.sol"; import {ExecutionInstallDelegate} from "../helpers/ExecutionInstallDelegate.sol"; import {SignatureType} from "../helpers/SignatureType.sol"; import {RTCallBuffer, SigCallBuffer, UOCallBuffer} from "../libraries/ExecutionLib.sol"; @@ -24,6 +25,7 @@ import {ModularAccountBase} from "./ModularAccountBase.sol"; abstract contract SemiModularAccountBase is ModularAccountBase { using MessageHashUtils for bytes32; using ModuleEntityLib for ModuleEntity; + using ValidationConfigLib for ValidationConfig; struct SemiModularAccountStorage { address fallbackSigner; @@ -44,6 +46,7 @@ abstract contract SemiModularAccountBase is ModularAccountBase { event FallbackSignerUpdated(address indexed newFallbackSigner, bool isDisabled); error FallbackSignerMismatch(); + error FallbackValidationInstallationNotAllowed(); error FallbackSignerDisabled(); error InvalidSignatureType(); @@ -63,6 +66,24 @@ abstract contract SemiModularAccountBase is ModularAccountBase { emit FallbackSignerUpdated(fallbackSigner, isDisabled); } + function installValidation( + ValidationConfig validationConfig, + bytes4[] calldata selectors, + bytes calldata installData, + bytes[] calldata hooks + ) external override wrapNativeFunction { + // Previously, it was possible to "alias" the fallback validation by installing a module at the reserved + // validation entity id 0. Not failing here could cause unexpected behavior, so this is checked to + // explicitly revert and warn the caller that this operation would not do what is requested. + // + // Note that this state can still be reached by upgrading from MA to SMA, but should be handled with + // initialization and de-init steps. + if (validationConfig.entityId() == FALLBACK_VALIDATION_ID && validationConfig.module() != address(0)) { + revert FallbackValidationInstallationNotAllowed(); + } + _installValidation(validationConfig, selectors, installData, hooks); + } + /// @notice Returns the fallback signer data in storage. /// @return The fallback signer and a boolean, true if the fallback signer validation is disabled, false if it /// is enabled. @@ -72,12 +93,12 @@ abstract contract SemiModularAccountBase is ModularAccountBase { } function _execUserOpValidation( - ModuleEntity userOpValidationFunction, + ValidationLookupKey validationLookupKey, bytes32 userOpHash, bytes calldata signatureSegment, UOCallBuffer callBuffer ) internal override returns (uint256) { - if (userOpValidationFunction.eq(FALLBACK_VALIDATION)) { + if (validationLookupKey.eq(FALLBACK_VALIDATION_LOOKUP_KEY)) { address fallbackSigner = _getFallbackSigner(); if (_checkSignature(fallbackSigner, userOpHash.toEthSignedMessageHash(), signatureSegment)) { @@ -86,32 +107,32 @@ abstract contract SemiModularAccountBase is ModularAccountBase { return _SIG_VALIDATION_FAILED; } - return super._execUserOpValidation(userOpValidationFunction, userOpHash, signatureSegment, callBuffer); + return super._execUserOpValidation(validationLookupKey, userOpHash, signatureSegment, callBuffer); } function _execRuntimeValidation( - ModuleEntity runtimeValidationFunction, + ValidationLookupKey validationLookupKey, RTCallBuffer callBuffer, bytes calldata authorization ) internal override { - if (runtimeValidationFunction.eq(FALLBACK_VALIDATION)) { + if (validationLookupKey.eq(FALLBACK_VALIDATION_LOOKUP_KEY)) { address fallbackSigner = _getFallbackSigner(); if (msg.sender != fallbackSigner) { revert FallbackSignerMismatch(); } } else { - super._execRuntimeValidation(runtimeValidationFunction, callBuffer, authorization); + super._execRuntimeValidation(validationLookupKey, callBuffer, authorization); } } function _exec1271Validation( SigCallBuffer buffer, bytes32 hash, - ModuleEntity sigValidation, + ValidationLookupKey validationLookupKey, bytes calldata signature ) internal view override returns (bytes4) { - if (sigValidation.eq(FALLBACK_VALIDATION)) { + if (validationLookupKey.eq(FALLBACK_VALIDATION_LOOKUP_KEY)) { address fallbackSigner = _getFallbackSigner(); // If called during validateUserOp, this implies that we're doing a deferred validation installation. @@ -125,7 +146,7 @@ abstract contract SemiModularAccountBase is ModularAccountBase { } return _1271_INVALID; } - return super._exec1271Validation(buffer, hash, sigValidation, signature); + return super._exec1271Validation(buffer, hash, validationLookupKey, signature); } function _checkSignature(address owner, bytes32 digest, bytes calldata sig) internal view returns (bool) { @@ -233,8 +254,14 @@ abstract contract SemiModularAccountBase is ModularAccountBase { } // Conditionally skip allocation of call buffers. - function _validationIsNative(ModuleEntity validationFunction) internal pure virtual override returns (bool) { - return validationFunction.eq(FALLBACK_VALIDATION); + function _validationIsNative(ValidationLookupKey validationLookupKey) + internal + pure + virtual + override + returns (bool) + { + return validationLookupKey.eq(FALLBACK_VALIDATION_LOOKUP_KEY); } /// @notice Adds a EIP-712 replay safe hash wrapper to the digest diff --git a/src/helpers/Constants.sol b/src/helpers/Constants.sol index 79dd95be..fc71fa80 100644 --- a/src/helpers/Constants.sol +++ b/src/helpers/Constants.sol @@ -8,5 +8,8 @@ import {ValidationLookupKey} from "../libraries/ValidationLocatorLib.sol"; // Magic value for the ModuleEntity of the fallback validation for SemiModularAccount. ModuleEntity constant FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(0)); +// Magic value for the validation entity id of the fallback validation for SemiModularAccount. +uint32 constant FALLBACK_VALIDATION_ID = uint32(0); + // Magic value for the ValidationLookupKey of the fallback validation for SemiModularAccount. ValidationLookupKey constant FALLBACK_VALIDATION_LOOKUP_KEY = ValidationLookupKey.wrap(uint168(0)); diff --git a/src/helpers/NativeFunctionDelegate.sol b/src/helpers/NativeFunctionDelegate.sol index 06a60f62..8e25fc72 100644 --- a/src/helpers/NativeFunctionDelegate.sol +++ b/src/helpers/NativeFunctionDelegate.sol @@ -33,7 +33,6 @@ contract NativeFunctionDelegate { || selector == uint32(IModularAccount.accountId.selector) || selector == uint32(IAccountExecute.executeUserOp.selector) || selector == uint32(IModularAccountBase.performCreate.selector) - || selector == uint32(IModularAccountBase.invalidateDeferredValidationInstallNonce.selector) || selector == uint32(IERC1271.isValidSignature.selector) // check against IModularAccountView methods || selector == uint32(IModularAccountView.getExecutionData.selector) diff --git a/src/interfaces/IModularAccountBase.sol b/src/interfaces/IModularAccountBase.sol index 802e3e71..e8df1039 100644 --- a/src/interfaces/IModularAccountBase.sol +++ b/src/interfaces/IModularAccountBase.sol @@ -12,8 +12,4 @@ interface IModularAccountBase { external payable returns (address createdAddr); - - /// @notice Invalidate a nonce for deferred actions - /// @param nonce the nonce to invalidate. - function invalidateDeferredValidationInstallNonce(uint256 nonce) external; } diff --git a/src/libraries/ValidationLocatorLib.sol b/src/libraries/ValidationLocatorLib.sol index 7ec041b7..ec30c7ca 100644 --- a/src/libraries/ValidationLocatorLib.sol +++ b/src/libraries/ValidationLocatorLib.sol @@ -38,15 +38,15 @@ library ValidationLocatorLib { uint8 internal constant _HAS_DEFERRED_ACTION = 2; uint8 internal constant _IS_DIRECT_CALL_VALIDATION = 4; - function moduleEntity(ValidationLocator locator, ValidationStorage storage validationStorage) + function moduleEntity(ValidationLookupKey _lookupKey, ValidationStorage storage validationStorage) internal view returns (ModuleEntity result) { - if (locator.isDirectCallValidation()) { - result = ModuleEntityLib.pack(locator.directCallAddress(), DIRECT_CALL_VALIDATION_ENTITYID); + if (_lookupKey.isDirectCallValidation()) { + result = ModuleEntityLib.pack(_lookupKey.directCallAddress(), DIRECT_CALL_VALIDATION_ENTITYID); } else { - result = ModuleEntityLib.pack(validationStorage.module, locator.entityId()); + result = ModuleEntityLib.pack(validationStorage.module, _lookupKey.entityId()); } } @@ -157,17 +157,17 @@ library ValidationLocatorLib { } } - // Only safe to call if the locator has been asserted to be a direct call validation. - function directCallAddress(ValidationLocator locator) internal pure returns (address result) { + // Only safe to call if the lookup has been asserted to be a direct call validation. + function directCallAddress(ValidationLookupKey _lookupKey) internal pure returns (address result) { assembly ("memory-safe") { - result := and(shr(8, locator), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) + result := and(shr(8, _lookupKey), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) } } - // Only safe to call if the locator has been asserted to be a non-direct call validation. - function entityId(ValidationLocator locator) internal pure returns (uint32 result) { + // Only safe to call if the lookup has been asserted to be a non-direct call validation. + function entityId(ValidationLookupKey _lookupKey) internal pure returns (uint32 result) { assembly ("memory-safe") { - result := and(shr(8, locator), 0xFFFFFFFFFFFFFFFF) + result := and(shr(8, _lookupKey), 0xFFFFFFFFFFFFFFFF) } } @@ -179,8 +179,8 @@ library ValidationLocatorLib { return (ValidationLocator.unwrap(locator) & _HAS_DEFERRED_ACTION) != 0; } - function isDirectCallValidation(ValidationLocator locator) internal pure returns (bool) { - return (ValidationLocator.unwrap(locator) & _IS_DIRECT_CALL_VALIDATION) != 0; + function isDirectCallValidation(ValidationLookupKey _lookupKey) internal pure returns (bool) { + return (ValidationLookupKey.unwrap(_lookupKey) & _IS_DIRECT_CALL_VALIDATION) != 0; } function configToLookupKey(ValidationConfig validationConfig) @@ -324,6 +324,32 @@ library ValidationLocatorLib { return bytes.concat(abi.encodePacked(options, uint160(directCallValidation)), signature); } + // Converts a module entity to a locator. + function packFromModuleEntity(ModuleEntity _moduleEntity, bool _isGlobal, bool _hasDeferredAction) + internal + pure + returns (ValidationLocator) + { + uint168 result; + + (address module, uint32 _entityId) = ModuleEntityLib.unpack(_moduleEntity); + if (_entityId == DIRECT_CALL_VALIDATION_ENTITYID) { + result = uint168(uint160(module)) << 8 | _IS_DIRECT_CALL_VALIDATION; + } else { + result = uint168(_entityId) << 8; + } + + if (_isGlobal) { + result |= _VALIDATION_TYPE_GLOBAL; + } + + if (_hasDeferredAction) { + result |= _HAS_DEFERRED_ACTION; + } + + return ValidationLocator.wrap(result); + } + // Operators function eq(ValidationLookupKey a, ValidationLookupKey b) internal pure returns (bool) { diff --git a/test/account/DeferredAction.t.sol b/test/account/DeferredAction.t.sol index 42688509..bebae612 100644 --- a/test/account/DeferredAction.t.sol +++ b/test/account/DeferredAction.t.sol @@ -99,7 +99,6 @@ contract DeferredActionTest is AccountTestBase { (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; bytes memory deferredAction = abi.encodeCall( @@ -108,14 +107,7 @@ contract DeferredActionTest is AccountTestBase { ); userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - deferredAction, - // Use the same validation for the deferred action and the user op - ValidationConfigLib.pack(_signerValidation, true, false, false), - account2, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, deferredAction, account2, owner1Key, uoSig ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); @@ -160,20 +152,12 @@ contract DeferredActionTest is AccountTestBase { (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; bytes memory deferredAction = abi.encodeWithSelector(newFunctionSelector); userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - deferredAction, - // Use the same validation for the deferred action and the user op - ValidationConfigLib.pack(_signerValidation, true, false, false), - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, deferredAction, account1, owner1Key, uoSig ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); @@ -210,7 +194,6 @@ contract DeferredActionTest is AccountTestBase { (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; bytes memory deferredAction = abi.encodeCall( @@ -218,14 +201,7 @@ contract DeferredActionTest is AccountTestBase { ); userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - deferredAction, - // Use the same validation for the deferred action and the user op - ValidationConfigLib.pack(_signerValidation, true, false, false), - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, deferredAction, account1, owner1Key, uoSig ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); @@ -262,7 +238,6 @@ contract DeferredActionTest is AccountTestBase { (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; Call[] memory innerCalls = new Call[](1); @@ -279,14 +254,7 @@ contract DeferredActionTest is AccountTestBase { bytes memory deferredAction = abi.encodeCall(IModularAccount.executeBatch, (outerCalls)); userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - deferredAction, - // Use the same validation for the deferred action and the user op - ValidationConfigLib.pack(_signerValidation, true, false, false), - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, deferredAction, account1, owner1Key, uoSig ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); @@ -353,20 +321,12 @@ contract DeferredActionTest is AccountTestBase { (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; bytes memory deferredAction = abi.encodeCall(IModularAccount.execute, (address(0), 0 wei, hex"02")); userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - deferredAction, - // Use the same validation for the deferred action and the user op - ValidationConfigLib.pack(validation2, true, false, false), - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, deferredAction, account1, owner1Key, uoSig ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); diff --git a/test/account/DeferredValidation.t.sol b/test/account/DeferredValidation.t.sol index 1614ccd4..0e6ee286 100644 --- a/test/account/DeferredValidation.t.sol +++ b/test/account/DeferredValidation.t.sol @@ -90,27 +90,16 @@ contract DeferredValidationTest is AccountTestBase { vm.sign(_newSignerKey, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - _deferredValidationInstallCall, - _newUOValidation, - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, _deferredValidationInstallCall, account1, owner1Key, uoSig ); _sendOp(userOp, ""); - bytes memory expectedRevertData = abi.encodeWithSelector( - IEntryPoint.FailedOpWithRevert.selector, - 0, - "AA23 reverted", - abi.encodeWithSelector(ModularAccountBase.DeferredActionNonceInvalid.selector) - ); + bytes memory expectedRevertData = + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA25 invalid account nonce"); _sendOp(userOp, expectedRevertData); } @@ -136,17 +125,10 @@ contract DeferredValidationTest is AccountTestBase { vm.sign(_newSignerKey, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 1; userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - _deferredValidationInstallCall, - _newUOValidation, - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, _deferredValidationInstallCall, account1, owner1Key, uoSig ); bytes memory expectedRevertData = @@ -173,19 +155,12 @@ contract DeferredValidationTest is AccountTestBase { vm.sign(_newSignerKey, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; (, uint256 badSigningKey) = makeAddrAndKey("bad"); userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - _deferredValidationInstallCall, - _newUOValidation, - account1, - badSigningKey, - uoSig + userOp.nonce, deferredInstallDeadline, _deferredValidationInstallCall, account1, badSigningKey, uoSig ); bytes memory expectedRevertData = abi.encodeWithSelector( @@ -199,9 +174,6 @@ contract DeferredValidationTest is AccountTestBase { } function test_fail_deferredValidation_nonceInvalidated() external withSMATest { - vm.prank(address(entryPoint)); - account1.invalidateDeferredValidationInstallNonce(0); - PackedUserOperation memory userOp = PackedUserOperation({ sender: address(account1), nonce: _encodeNextNonce(address(account1), _deferredValidation, true, true), @@ -217,27 +189,20 @@ contract DeferredValidationTest is AccountTestBase { bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(_newSignerKey, MessageHashUtils.toEthSignedMessageHash(userOpHash)); - bytes memory uoSig = _packFinalSignature(abi.encodePacked(r, s, v)); + bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - _deferredValidationInstallCall, - _newUOValidation, - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, _deferredValidationInstallCall, account1, owner1Key, uoSig ); - bytes memory expectedRevertData = abi.encodeWithSelector( - IEntryPoint.FailedOpWithRevert.selector, - 0, - "AA23 reverted", - abi.encodeWithSelector(ModularAccountBase.DeferredActionNonceInvalid.selector) - ); + // Invalidate the user op nonce (also deferred action nonce) + vm.prank(address(account1)); + entryPoint.incrementNonce(uint192(userOp.nonce >> 64)); + + bytes memory expectedRevertData = + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA25 invalid account nonce"); _sendOp(userOp, expectedRevertData); } @@ -260,17 +225,10 @@ contract DeferredValidationTest is AccountTestBase { bytes32 r = keccak256("invalid"); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - _deferredValidationInstallCall, - _newUOValidation, - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, _deferredValidationInstallCall, account1, owner1Key, uoSig ); bytes memory expectedRevertData = @@ -308,17 +266,10 @@ contract DeferredValidationTest is AccountTestBase { vm.sign(_newSignerKey, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - _deferredValidationInstallCall, - _newUOValidation, - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, _deferredValidationInstallCall, account1, owner1Key, uoSig ); bytes memory expectedRevertData = abi.encodeWithSelector( @@ -350,17 +301,10 @@ contract DeferredValidationTest is AccountTestBase { vm.sign(_newSignerKey, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - _deferredValidationInstallCall, - _newUOValidation, - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, _deferredValidationInstallCall, account1, owner1Key, uoSig ); _sendOp(userOp, ""); @@ -397,17 +341,10 @@ contract DeferredValidationTest is AccountTestBase { vm.sign(_newSignerKey, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - _deferredValidationInstallCall, - _newUOValidation, - account1, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, _deferredValidationInstallCall, account1, owner1Key, uoSig ); _sendOp(userOp, ""); @@ -453,17 +390,10 @@ contract DeferredValidationTest is AccountTestBase { vm.sign(_newSignerKey, MessageHashUtils.toEthSignedMessageHash(userOpHash)); bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); - uint256 deferredInstallNonce = 0; uint48 deferredInstallDeadline = 0; userOp.signature = _buildFullDeferredInstallSig( - deferredInstallNonce, - deferredInstallDeadline, - _deferredValidationInstallCall, - _newUOValidation, - account2, - owner1Key, - uoSig + userOp.nonce, deferredInstallDeadline, _deferredValidationInstallCall, account2, owner1Key, uoSig ); _sendOp(userOp, ""); diff --git a/test/account/PerHookData.t.sol b/test/account/PerHookData.t.sol index 493954ff..9b5442f6 100644 --- a/test/account/PerHookData.t.sol +++ b/test/account/PerHookData.t.sol @@ -40,7 +40,7 @@ contract PerHookDataTest is CustomValidationTestBase { Counter internal _counter; - uint32 internal constant _VALIDATION_ENTITY_ID = 0; + uint32 internal constant _VALIDATION_ENTITY_ID = 1; uint32 internal constant _PRE_HOOK_ENTITY_ID_1 = 0; uint32 internal constant _PRE_HOOK_ENTITY_ID_2 = 1; @@ -53,7 +53,10 @@ contract PerHookDataTest is CustomValidationTestBase { _customValidationSetup(); } - function test_passAccessControl_userOp() public withSMATest { + function test_passAccessControl_userOp() public { + _switchToSMA(); + setUp(); + assertEq(_counter.number(), 0); (PackedUserOperation memory userOp, bytes32 userOpHash) = _getCounterUserOP(); diff --git a/test/account/UpgradeToSma.t.sol b/test/account/UpgradeToSma.t.sol index e87d3604..3cca12c0 100644 --- a/test/account/UpgradeToSma.t.sol +++ b/test/account/UpgradeToSma.t.sol @@ -71,8 +71,9 @@ contract UpgradeToSmaTest is AccountTestBase { smaStorageImpl, abi.encodeCall(SemiModularAccountBase.updateFallbackSignerData, (owner2, false)) ); - // The previous owner1 validation is still installed, so this should not revert. - _userOpTransfer(address(account1), owner1Key, "", 0, false); + // This case is also unreachable, because the fallback validation aliases the original signer's validation. + // // The previous owner1 validation is still installed, so this should not revert. + // _userOpTransfer(address(account1), owner1Key, "", 0, false); vm.prank(owner2); account1.uninstallValidation(_signerValidation, "", new bytes[](0)); @@ -96,10 +97,10 @@ contract UpgradeToSmaTest is AccountTestBase { bytes memory expectedRevertdata = abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error"); // Execute a UO with the original signer and the fallback validation, anticipating a revert. - _userOpTransfer(address(account1), owner1Key, expectedRevertdata, transferAmount, true); + _userOpTransfer(address(account1), owner1Key, expectedRevertdata, 0, true); // Execute a UO with the new signer, which is the fallback signer. - _userOpTransfer(address(account1), owner2Key, "", transferAmount, true); + _userOpTransfer(address(account1), owner2Key, "", 0, true); } function test_upgradeToAndCall_LaToSmaStorage() external { diff --git a/test/modules/PaymasterGuardModule.t.sol b/test/modules/PaymasterGuardModule.t.sol index a7810e14..36d60a1b 100644 --- a/test/modules/PaymasterGuardModule.t.sol +++ b/test/modules/PaymasterGuardModule.t.sol @@ -40,7 +40,7 @@ contract PaymasterGuardModuleTest is AccountTestBase { address public account; address public paymaster1; address public paymaster2; - uint32 public constant ENTITY_ID = TEST_DEFAULT_VALIDATION_ENTITY_ID; + uint32 public constant ENTITY_ID = 1; function setUp() public override { _revertSnapshot = vm.snapshotState(); diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 80ef52d9..be99bee0 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -35,6 +35,7 @@ import {SemiModularAccountBytecode} from "../../src/account/SemiModularAccountBy import {AccountFactory} from "../../src/factory/AccountFactory.sol"; import {FALLBACK_VALIDATION} from "../../src/helpers/Constants.sol"; import {ExecutionInstallDelegate} from "../../src/helpers/ExecutionInstallDelegate.sol"; +import {ValidationLocator, ValidationLocatorLib} from "../../src/libraries/ValidationLocatorLib.sol"; import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol"; import {WebAuthnValidationModule} from "../../src/modules/validation/WebAuthnValidationModule.sol"; @@ -316,24 +317,24 @@ abstract contract AccountTestBase is OptimizedTest, ModuleSignatureUtils { // - ecdsaValidation, when not SMA // for 1271 signing the deferred action of install function _buildFullDeferredInstallSig( - uint256 deferredInstallNonce, + uint256 userOpNonce, uint48 deferredInstallDeadline, bytes memory deferredValidationInstallCall, - ValidationConfig uoValidationFunction, ModularAccount account, uint256 signingKey, bytes memory uoSig ) internal view returns (bytes memory) { + ValidationLocator defActionValidation = ValidationLocatorLib.packFromModuleEntity({ + _moduleEntity: _signerValidation, + _isGlobal: true, + _hasDeferredAction: false // The inner deferred action can't recursively have deferred actions. + }); + bytes memory deferredValidationSig; bytes memory deferredValidationDatas; { bytes32 digest = _getDeferredInstallStruct( - account, - deferredInstallNonce, - deferredInstallDeadline, - // Use global validation for performing the deferred action. - uoValidationFunction, - deferredValidationInstallCall + account, userOpNonce, deferredInstallDeadline, deferredValidationInstallCall ); bytes32 replaySafeHash; @@ -347,10 +348,7 @@ abstract contract AccountTestBase is OptimizedTest, ModuleSignatureUtils { deferredValidationSig = _signRawHash(vm, signingKey, replaySafeHash); deferredValidationDatas = _packDeferredInstallData( - deferredInstallNonce, - deferredInstallDeadline, - ValidationConfigLib.pack(_signerValidation, true, false, false), - deferredValidationInstallCall + deferredInstallDeadline, defActionValidation, deferredValidationInstallCall ); } diff --git a/test/utils/ModuleSignatureUtils.sol b/test/utils/ModuleSignatureUtils.sol index 4a10e67e..5ba229f0 100644 --- a/test/utils/ModuleSignatureUtils.sol +++ b/test/utils/ModuleSignatureUtils.sol @@ -25,10 +25,6 @@ import { } from "@erc6900/reference-implementation/helpers/Constants.sol"; import {ModuleEntity} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; import {ModuleEntityLib} from "@erc6900/reference-implementation/libraries/ModuleEntityLib.sol"; -import { - ValidationConfig, - ValidationConfigLib -} from "@erc6900/reference-implementation/libraries/ValidationConfigLib.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {ModularAccount} from "../../src/account/ModularAccount.sol"; @@ -59,9 +55,8 @@ contract ModuleSignatureUtils { uint8 public constant EOA_TYPE_SIGNATURE = 0; string internal constant _DEFERRED_ACTION_CONTENTS_TYPE = - "DeferredAction(uint256 nonce,uint48 deadline,uint168 validationLocator,bytes call)"; - bytes32 private constant _DEFERRED_ACTION_TYPEHASH = - keccak256(abi.encodePacked(_DEFERRED_ACTION_CONTENTS_TYPE)); + "DeferredAction(uint256 nonce,uint48 deadline,bytes call)"; + bytes32 private immutable _DEFERRED_ACTION_TYPEHASH; bytes32 internal constant _REPLAY_SAFE_HASH_TYPEHASH = keccak256("ReplaySafeHash(bytes32 hash)"); @@ -70,6 +65,10 @@ contract ModuleSignatureUtils { bytes32 internal constant _MODULE_DOMAIN_SEPARATOR = keccak256("EIP712Domain(uint256 chainId,address verifyingContract,bytes32 salt)"); + constructor() { + _DEFERRED_ACTION_TYPEHASH = keccak256(abi.encodePacked(_DEFERRED_ACTION_CONTENTS_TYPE)); + } + function _encodeSignature(PreValidationHookData[] memory preValidationHookData, bytes memory validationData) internal pure @@ -287,38 +286,29 @@ contract ModuleSignatureUtils { ); } - function _packDeferredInstallData( - uint256 nonce, - uint48 deadline, - ValidationConfig validationFunction, - bytes memory call - ) internal pure returns (bytes memory) { - bytes memory deferredInstallData = abi.encodePacked(nonce, deadline, validationFunction, call); + function _packDeferredInstallData(uint48 deadline, ValidationLocator validationFunction, bytes memory call) + internal + pure + returns (bytes memory) + { + bytes memory deferredInstallData = abi.encodePacked(validationFunction, deadline, call); return deferredInstallData; } function _getDeferredInstallStruct( ModularAccount account, - uint256 nonce, + uint256 userOpNonce, uint48 deadline, - ValidationConfig validationFunction, bytes memory selfCall ) internal view returns (bytes32) { - // Assumes this is not using the direct call path, and that isGlobal is true. - ValidationLocator locator = ValidationLocatorLib.pack({ - _entityId: ValidationConfigLib.entityId(validationFunction), - _isGlobal: true, - _hasDeferredAction: true - }); - bytes32 domainSeparator = _computeDomainSeparator(address(account)); bytes32 selfCallHash = keccak256(selfCall); return MessageHashUtils.toTypedDataHash({ domainSeparator: domainSeparator, - structHash: keccak256(abi.encode(_DEFERRED_ACTION_TYPEHASH, nonce, deadline, locator, selfCallHash)) + structHash: keccak256(abi.encode(_DEFERRED_ACTION_TYPEHASH, userOpNonce, deadline, selfCallHash)) }); }