From f62dbb5bf9349a10e362bf02570868a3dc560b17 Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Fri, 22 Nov 2024 12:49:15 -0500 Subject: [PATCH] fix: [S7] don't return exec hooks in getExecutionData for native functions that don't execute msg.sig-based hooks --- gas-snapshots/ModularAccount.json | 18 +- gas-snapshots/SemiModularAccount.json | 18 +- src/account/ModularAccount.sol | 5 + src/account/ModularAccountBase.sol | 8 +- src/account/ModularAccountView.sol | 59 +++--- src/account/SemiModularAccountBase.sol | 14 +- src/account/SemiModularAccountStorageOnly.sol | 5 + src/helpers/NativeFunctionDelegate.sol | 7 +- test/account/ModularAccountView.t.sol | 174 +++++++++++++++++- 9 files changed, 246 insertions(+), 62 deletions(-) diff --git a/gas-snapshots/ModularAccount.json b/gas-snapshots/ModularAccount.json index 6e877df8..8e14e693 100644 --- a/gas-snapshots/ModularAccount.json +++ b/gas-snapshots/ModularAccount.json @@ -1,16 +1,16 @@ { "Runtime_AccountCreation": "176125", - "Runtime_BatchTransfers": "93045", - "Runtime_Erc20Transfer": "78454", - "Runtime_InstallSessionKey_Case1": "423148", - "Runtime_NativeTransfer": "54603", + "Runtime_BatchTransfers": "93067", + "Runtime_Erc20Transfer": "78476", + "Runtime_InstallSessionKey_Case1": "423118", + "Runtime_NativeTransfer": "54625", "Runtime_UseSessionKey_Case1_Counter": "78606", "Runtime_UseSessionKey_Case1_Token": "111919", - "UserOp_BatchTransfers": "198366", - "UserOp_Erc20Transfer": "185331", - "UserOp_InstallSessionKey_Case1": "531355", - "UserOp_NativeTransfer": "161588", + "UserOp_BatchTransfers": "198388", + "UserOp_Erc20Transfer": "185353", + "UserOp_InstallSessionKey_Case1": "531325", + "UserOp_NativeTransfer": "161610", "UserOp_UseSessionKey_Case1_Counter": "195233", "UserOp_UseSessionKey_Case1_Token": "226217", - "UserOp_deferredValidation": "258080" + "UserOp_deferredValidation": "258072" } \ No newline at end of file diff --git a/gas-snapshots/SemiModularAccount.json b/gas-snapshots/SemiModularAccount.json index 2e9dfa3e..05465403 100644 --- a/gas-snapshots/SemiModularAccount.json +++ b/gas-snapshots/SemiModularAccount.json @@ -1,16 +1,16 @@ { "Runtime_AccountCreation": "97745", - "Runtime_BatchTransfers": "89031", - "Runtime_Erc20Transfer": "74488", - "Runtime_InstallSessionKey_Case1": "421700", - "Runtime_NativeTransfer": "50647", + "Runtime_BatchTransfers": "89008", + "Runtime_Erc20Transfer": "74465", + "Runtime_InstallSessionKey_Case1": "421625", + "Runtime_NativeTransfer": "50624", "Runtime_UseSessionKey_Case1_Counter": "78956", "Runtime_UseSessionKey_Case1_Token": "112269", - "UserOp_BatchTransfers": "193522", - "UserOp_Erc20Transfer": "180558", - "UserOp_InstallSessionKey_Case1": "528878", - "UserOp_NativeTransfer": "156833", + "UserOp_BatchTransfers": "193499", + "UserOp_Erc20Transfer": "180535", + "UserOp_InstallSessionKey_Case1": "528803", + "UserOp_NativeTransfer": "156810", "UserOp_UseSessionKey_Case1_Counter": "195473", "UserOp_UseSessionKey_Case1_Token": "226457", - "UserOp_deferredValidation": "254131" + "UserOp_deferredValidation": "254033" } \ No newline at end of file diff --git a/src/account/ModularAccount.sol b/src/account/ModularAccount.sol index bb2d7389..3ff2e5dc 100644 --- a/src/account/ModularAccount.sol +++ b/src/account/ModularAccount.sol @@ -30,4 +30,9 @@ contract ModularAccount is ModularAccountBase { function accountId() external pure override returns (string memory) { return "alchemy.modular-account.2.0.0"; } + + /// @dev Overrides ModularAccountView. + function _isNativeFunction(uint32 selector) internal view override returns (bool) { + return super._isNativeFunction(selector) || selector == uint32(this.initializeWithValidation.selector); + } } diff --git a/src/account/ModularAccountBase.sol b/src/account/ModularAccountBase.sol index cfe05cf4..2dbb5acc 100644 --- a/src/account/ModularAccountBase.sol +++ b/src/account/ModularAccountBase.sol @@ -110,8 +110,8 @@ abstract contract ModularAccountBase is error DeferredValidationHasValidationHooks(); // Wraps execution of a native function with runtime validation and hooks - // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installExecution, uninstallExecution, - // performCreate + // Used for performCreate, execute, executeBatch, installExecution, uninstallExecution, installValidation, + // uninstallValidation, invalidateDeferredValidationInstallNonce, upgradeToAndCall, updateFallbackSignerData. modifier wrapNativeFunction() { DensePostHookData postHookData = _checkPermittedCallerAndAssociatedHooks(); @@ -1102,8 +1102,8 @@ abstract contract ModularAccountBase is return _globalValidationAllowed(selector) && _isValidationGlobal(validationFunction); } - function _globalValidationAllowed(bytes4 selector) internal view virtual returns (bool) { - return _isGlobalValidationAllowedNativeFunction(selector) + function _globalValidationAllowed(bytes4 selector) internal view returns (bool) { + return _isGlobalValidationAllowedNativeFunction(uint32(selector)) || getAccountStorage().executionStorage[selector].allowGlobalValidation; } diff --git a/src/account/ModularAccountView.sol b/src/account/ModularAccountView.sol index c7940e14..9ce5a635 100644 --- a/src/account/ModularAccountView.sol +++ b/src/account/ModularAccountView.sol @@ -2,16 +2,17 @@ pragma solidity ^0.8.26; import {HookConfig, ModuleEntity} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; +import {IModularAccount} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; import { ExecutionDataView, IModularAccountView, ValidationDataView } from "@erc6900/reference-implementation/interfaces/IModularAccountView.sol"; - -import {IModularAccount} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; import {IModularAccountView} from "@erc6900/reference-implementation/interfaces/IModularAccountView.sol"; import {IAccountExecute} from "@eth-infinitism/account-abstraction/interfaces/IAccountExecute.sol"; +import {IERC1155Receiver} from "@openzeppelin/contracts/interfaces/IERC1155Receiver.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; +import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol"; import {NativeFunctionDelegate} from "../helpers/NativeFunctionDelegate.sol"; import {IModularAccountBase} from "../interfaces/IModularAccountBase.sol"; @@ -31,16 +32,18 @@ abstract contract ModularAccountView is IModularAccountView { /// @inheritdoc IModularAccountView function getExecutionData(bytes4 selector) external view override returns (ExecutionDataView memory data) { - // return ModularAccountViewLib.getExecutionData(selector, _isNativeFunction(selector)); ExecutionStorage storage executionStorage = getAccountStorage().executionStorage[selector]; - if (_isGlobalValidationAllowedNativeFunction(selector)) { - data.module = address(this); - data.allowGlobalValidation = true; - } else if (_isNativeFunction(uint32(selector))) { - // native view functions + if (_isNativeFunction(uint32(selector))) { + bool isGlobalValidationAllowed = _isGlobalValidationAllowedNativeFunction(uint32(selector)); data.module = address(this); - data.skipRuntimeValidation = true; + data.skipRuntimeValidation = !isGlobalValidationAllowed; + data.allowGlobalValidation = isGlobalValidationAllowed; + if (!_isWrappedNativeFunction(uint32(selector))) { + // The native function does not run execution hooks associated with its selector, so + // we can return early. + return data; + } } else { data.module = executionStorage.module; data.skipRuntimeValidation = executionStorage.skipRuntimeValidation; @@ -79,21 +82,27 @@ abstract contract ModularAccountView is IModularAccountView { return _NATIVE_FUNCTION_DELEGATE.isNativeFunction(selector); } - function _isGlobalValidationAllowedNativeFunction(bytes4 selector) internal view virtual returns (bool) { - if ( - selector == IModularAccount.execute.selector || selector == IModularAccount.executeBatch.selector - || selector == IAccountExecute.executeUserOp.selector - || selector == IModularAccount.executeWithRuntimeValidation.selector - || selector == IModularAccount.installExecution.selector - || selector == IModularAccount.uninstallExecution.selector - || selector == IModularAccount.installValidation.selector - || selector == IModularAccount.uninstallValidation.selector - || selector == UUPSUpgradeable.upgradeToAndCall.selector - || selector == IModularAccountBase.invalidateDeferredValidationInstallNonce.selector - || selector == IModularAccountBase.performCreate.selector - ) { - return true; - } - return false; + /// @dev Check whether a function is a native function that has the `wrapNativeFunction` modifier applied, + /// which means it runs execution hooks associated with its selector. + function _isWrappedNativeFunction(uint32 selector) internal pure virtual returns (bool) { + return ( + selector == uint32(IModularAccount.execute.selector) + || selector == uint32(IModularAccount.executeBatch.selector) + || selector == uint32(IModularAccount.installExecution.selector) + || selector == uint32(IModularAccount.uninstallExecution.selector) + || selector == uint32(IModularAccount.installValidation.selector) + || selector == uint32(IModularAccount.uninstallValidation.selector) + || selector == uint32(UUPSUpgradeable.upgradeToAndCall.selector) + || selector == uint32(IModularAccountBase.invalidateDeferredValidationInstallNonce.selector) + || selector == uint32(IModularAccountBase.performCreate.selector) + ); + } + + /// @dev Check whether a function is a native function that allows global validation. + function _isGlobalValidationAllowedNativeFunction(uint32 selector) internal pure virtual returns (bool) { + return ( + _isWrappedNativeFunction(selector) || selector == uint32(IAccountExecute.executeUserOp.selector) + || selector == uint32(IModularAccount.executeWithRuntimeValidation.selector) + ); } } diff --git a/src/account/SemiModularAccountBase.sol b/src/account/SemiModularAccountBase.sol index d85c3e0f..4a098a0b 100644 --- a/src/account/SemiModularAccountBase.sol +++ b/src/account/SemiModularAccountBase.sol @@ -144,10 +144,6 @@ abstract contract SemiModularAccountBase is ModularAccountBase { revert InvalidSignatureType(); } - function _globalValidationAllowed(bytes4 selector) internal view override returns (bool) { - return selector == this.updateFallbackSignerData.selector || super._globalValidationAllowed(selector); - } - function _isValidationGlobal(ModuleEntity validationFunction) internal view override returns (bool) { if (validationFunction.eq(FALLBACK_VALIDATION) || super._isValidationGlobal(validationFunction)) { return true; @@ -198,12 +194,18 @@ abstract contract SemiModularAccountBase is ModularAccountBase { return _storage.fallbackSigner; } - // overrides ModularAccountView - function _isNativeFunction(uint32 selector) internal view override returns (bool) { + /// @dev Overrides ModularAccountView. + function _isNativeFunction(uint32 selector) internal view virtual override returns (bool) { return super._isNativeFunction(selector) || selector == uint32(this.updateFallbackSignerData.selector) || selector == uint32(this.getFallbackSignerData.selector); } + /// @dev Overrides ModularAccountView. + function _isWrappedNativeFunction(uint32 selector) internal pure virtual override returns (bool) { + return + super._isWrappedNativeFunction(selector) || selector == uint32(this.updateFallbackSignerData.selector); + } + /// @notice Returns the replay-safe hash generated from the passed typed data hash for 1271 validation. /// @param hash The typed data hash to wrap in a replay-safe hash. /// @return The replay-safe hash, to be used for 1271 signature generation. diff --git a/src/account/SemiModularAccountStorageOnly.sol b/src/account/SemiModularAccountStorageOnly.sol index 2082b953..af62fb99 100644 --- a/src/account/SemiModularAccountStorageOnly.sol +++ b/src/account/SemiModularAccountStorageOnly.sol @@ -29,4 +29,9 @@ contract SemiModularAccountStorageOnly is SemiModularAccountBase { function accountId() external pure override returns (string memory) { return "alchemy.sma-storage.1.0.0"; } + + /// @dev Overrides SemiModularAccountBase. + function _isNativeFunction(uint32 selector) internal view override returns (bool) { + return super._isNativeFunction(selector) || selector == uint32(this.initialize.selector); + } } diff --git a/src/helpers/NativeFunctionDelegate.sol b/src/helpers/NativeFunctionDelegate.sol index 0a596f36..fab39a9e 100644 --- a/src/helpers/NativeFunctionDelegate.sol +++ b/src/helpers/NativeFunctionDelegate.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.26; import {IModularAccount} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; import {IModularAccountView} from "@erc6900/reference-implementation/interfaces/IModularAccountView.sol"; -import {IAccount} from "@eth-infinitism/account-abstraction/interfaces/IAccount.sol"; import {IAccountExecute} from "@eth-infinitism/account-abstraction/interfaces/IAccountExecute.sol"; import {IERC1155Receiver} from "@openzeppelin/contracts/interfaces/IERC1155Receiver.sol"; import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; @@ -11,6 +10,7 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeab import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol"; import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {AccountBase} from "../account/AccountBase.sol"; import {IModularAccountBase} from "../interfaces/IModularAccountBase.sol"; /// @title Native Function Delegate @@ -20,8 +20,9 @@ import {IModularAccountBase} from "../interfaces/IModularAccountBase.sol"; contract NativeFunctionDelegate { function isNativeFunction(uint32 selector) external pure returns (bool) { return - // check against IAccount methods - selector == uint32(IAccount.validateUserOp.selector) + // check against AccountBase methods + selector == uint32(AccountBase.validateUserOp.selector) + || selector == uint32(AccountBase.entryPoint.selector) // check against ModularAccount methods || selector == uint32(IModularAccount.installExecution.selector) || selector == uint32(IModularAccount.uninstallExecution.selector) diff --git a/test/account/ModularAccountView.t.sol b/test/account/ModularAccountView.t.sol index 00f62199..ef0c7494 100644 --- a/test/account/ModularAccountView.t.sol +++ b/test/account/ModularAccountView.t.sol @@ -17,6 +17,11 @@ pragma solidity ^0.8.26; +import { + ExecutionManifest, + ManifestExecutionFunction, + ManifestExecutionHook +} from "@erc6900/reference-implementation/interfaces/IExecutionModule.sol"; import { HookConfig, IModularAccount, @@ -28,9 +33,14 @@ import { } from "@erc6900/reference-implementation/interfaces/IModularAccountView.sol"; import {HookConfigLib} from "@erc6900/reference-implementation/libraries/HookConfigLib.sol"; import {ModuleEntityLib} from "@erc6900/reference-implementation/libraries/ModuleEntityLib.sol"; +import {IAccountExecute} from "@eth-infinitism/account-abstraction/interfaces/IAccountExecute.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; +import {ModularAccount} from "../../src/account/ModularAccount.sol"; +import {SemiModularAccountBase} from "../../src/account/SemiModularAccountBase.sol"; +import {IModularAccountBase} from "../../src/interfaces/IModularAccountBase.sol"; import {ComprehensiveModule} from "../mocks/modules/ComprehensiveModule.sol"; +import {MockModule} from "../mocks/modules/MockModule.sol"; import {CustomValidationTestBase} from "../utils/CustomValidationTestBase.sol"; contract ModularAccountViewTest is CustomValidationTestBase { @@ -53,24 +63,176 @@ contract ModularAccountViewTest is CustomValidationTestBase { vm.stopPrank(); } - function test_moduleView_getExecutionData_native() public withSMATest { - bytes4[] memory selectorsToCheck = new bytes4[](5); + function test_moduleView_getExecutionData_nativeGlobalValidationAllowed() public withSMATest { + bytes4[] memory selectorsToCheck = new bytes4[](11); selectorsToCheck[0] = IModularAccount.execute.selector; - selectorsToCheck[1] = IModularAccount.executeBatch.selector; + selectorsToCheck[2] = IModularAccount.installExecution.selector; + selectorsToCheck[3] = IModularAccount.uninstallExecution.selector; + selectorsToCheck[4] = IModularAccount.installValidation.selector; + selectorsToCheck[5] = IModularAccount.uninstallValidation.selector; + selectorsToCheck[6] = UUPSUpgradeable.upgradeToAndCall.selector; + selectorsToCheck[7] = IModularAccountBase.invalidateDeferredValidationInstallNonce.selector; + selectorsToCheck[8] = IModularAccountBase.performCreate.selector; + selectorsToCheck[9] = IAccountExecute.executeUserOp.selector; + selectorsToCheck[10] = IModularAccount.executeWithRuntimeValidation.selector; + + for (uint256 i = 0; i < selectorsToCheck.length; i++) { + ExecutionDataView memory data = account1.getExecutionData(selectorsToCheck[i]); + assertEq(data.module, address(account1)); + assertTrue(data.allowGlobalValidation); + assertFalse(data.skipRuntimeValidation); + } + } + + function test_moduleView_getExecutionData_nativeGlobalValidationAllowedSMA() public withSMATest { + ExecutionDataView memory data = + account1.getExecutionData(SemiModularAccountBase.updateFallbackSignerData.selector); + + if (_isSMATest) { + assertEq(data.module, address(account1)); + assertTrue(data.allowGlobalValidation); + assertFalse(data.skipRuntimeValidation); + } else { + assertEq(data.module, address(0)); + assertFalse(data.allowGlobalValidation); + assertFalse(data.skipRuntimeValidation); + } + } + + /// @dev For native functions that do not have `wrapNativeFunction` modifier applied, hooks should not be + /// returned in the getExecutionData result since they will never run. We still allow installation for + /// compatibility reasons. + function test_moduleView_getExecutionData_nativeUnwrapped() public withSMATest { + ExecutionManifest memory manifest; + manifest.executionFunctions = new ManifestExecutionFunction[](1); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: IModularAccount.executeWithRuntimeValidation.selector, + // Flags here don't apply for native functions. They'll be stored, but unused, so getExecutionData + // should not return them and instead return the expected flags for the native function. + skipRuntimeValidation: false, + allowGlobalValidation: false + }); + manifest.executionHooks = new ManifestExecutionHook[](2); + // Hooks should not be returned in the getExecutionData result. + manifest.executionHooks[0] = ManifestExecutionHook({ + executionSelector: IModularAccount.executeWithRuntimeValidation.selector, + entityId: 0, + isPreHook: true, + isPostHook: false + }); + manifest.executionHooks[1] = ManifestExecutionHook({ + executionSelector: IAccountExecute.executeUserOp.selector, + entityId: 0, + isPreHook: true, + isPostHook: false + }); - selectorsToCheck[2] = UUPSUpgradeable.upgradeToAndCall.selector; + MockModule mockModule = new MockModule(manifest); + vm.startPrank(address(entryPoint)); + account1.installExecution(address(mockModule), mockModule.executionManifest(), ""); + vm.stopPrank(); - selectorsToCheck[3] = IModularAccount.installExecution.selector; + bytes4[] memory selectorsToCheck = new bytes4[](1); - selectorsToCheck[4] = IModularAccount.uninstallExecution.selector; + selectorsToCheck[0] = IModularAccount.executeWithRuntimeValidation.selector; for (uint256 i = 0; i < selectorsToCheck.length; i++) { ExecutionDataView memory data = account1.getExecutionData(selectorsToCheck[i]); assertEq(data.module, address(account1)); assertTrue(data.allowGlobalValidation); assertFalse(data.skipRuntimeValidation); + assertEq(data.executionHooks.length, 0); + } + } + + /// @dev For native functions that do not have `wrapNativeFunction` modifier applied, hooks should not be + /// returned in the getExecutionData result since they will never run. We still allow installation for + /// compatibility reasons. + function test_moduleView_getExecutionData_nativeUnwrappedSMA() public withSMATest { + ExecutionManifest memory manifest; + manifest.executionFunctions = new ManifestExecutionFunction[](1); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: SemiModularAccountBase.getFallbackSignerData.selector, + // Flags here don't apply for native functions. They'll be stored, but unused, so getExecutionData + // should not return them and instead return the expected flags for the native function. + skipRuntimeValidation: false, + allowGlobalValidation: false + }); + manifest.executionHooks = new ManifestExecutionHook[](1); + // Hooks should not be returned in the getExecutionData result. + manifest.executionHooks[0] = ManifestExecutionHook({ + executionSelector: SemiModularAccountBase.getFallbackSignerData.selector, + entityId: 0, + isPreHook: true, + isPostHook: false + }); + + MockModule mockModule = new MockModule(manifest); + vm.startPrank(address(entryPoint)); + account1.installExecution(address(mockModule), mockModule.executionManifest(), ""); + vm.stopPrank(); + + ExecutionDataView memory data = + account1.getExecutionData(SemiModularAccountBase.getFallbackSignerData.selector); + + if (_isSMATest) { + assertEq(data.module, address(account1)); + assertFalse(data.allowGlobalValidation); + assertTrue(data.skipRuntimeValidation); + assertEq(data.executionHooks.length, 0); + } else { + // If not SMA, this is not a native function, so the module should be the mock module, and all the + // flags and hooks should be consistent with the manifest. + assertEq(data.module, address(mockModule)); + assertFalse(data.allowGlobalValidation); + assertFalse(data.skipRuntimeValidation); + assertEq(data.executionHooks.length, 1); + } + } + + /// @dev For native functions that do not have `wrapNativeFunction` modifier applied, hooks should not be + /// returned in the getExecutionData result since they will never run. We still allow installation for + /// compatibility reasons. + function test_moduleView_getExecutionData_nativeUnwrappedMA() public withSMATest { + ExecutionManifest memory manifest; + manifest.executionFunctions = new ManifestExecutionFunction[](1); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: ModularAccount.initializeWithValidation.selector, + // Flags here don't apply for native functions. They'll be stored, but unused, so getExecutionData + // should not return them and instead return the expected flags for the native function. + skipRuntimeValidation: false, + allowGlobalValidation: false + }); + manifest.executionHooks = new ManifestExecutionHook[](1); + // Hooks should not be returned in the getExecutionData result. + manifest.executionHooks[0] = ManifestExecutionHook({ + executionSelector: ModularAccount.initializeWithValidation.selector, + entityId: 0, + isPreHook: true, + isPostHook: false + }); + + MockModule mockModule = new MockModule(manifest); + vm.startPrank(address(entryPoint)); + account1.installExecution(address(mockModule), mockModule.executionManifest(), ""); + vm.stopPrank(); + + ExecutionDataView memory data = account1.getExecutionData(ModularAccount.initializeWithValidation.selector); + + if (_isSMATest) { + // If not MA, this is not a native function, so the module should be the mock module, and all the + // flags and hooks should be consistent with the manifest. + assertEq(data.module, address(mockModule)); + assertFalse(data.allowGlobalValidation); + assertFalse(data.skipRuntimeValidation); + assertEq(data.executionHooks.length, 1); + } else { + assertEq(data.module, address(account1)); + assertFalse(data.allowGlobalValidation); + assertTrue(data.skipRuntimeValidation); + assertEq(data.executionHooks.length, 0); } }