diff --git a/gas-snapshots/ModularAccount.json b/gas-snapshots/ModularAccount.json index cf7c08be..e185ebb0 100644 --- a/gas-snapshots/ModularAccount.json +++ b/gas-snapshots/ModularAccount.json @@ -1,16 +1,16 @@ { "Runtime_AccountCreation": "176354", - "Runtime_BatchTransfers": "92886", - "Runtime_Erc20Transfer": "78318", - "Runtime_InstallSessionKey_Case1": "423362", - "Runtime_NativeTransfer": "54467", + "Runtime_BatchTransfers": "92908", + "Runtime_Erc20Transfer": "78340", + "Runtime_InstallSessionKey_Case1": "423306", + "Runtime_NativeTransfer": "54489", "Runtime_UseSessionKey_Case1_Counter": "78531", "Runtime_UseSessionKey_Case1_Token": "111844", - "UserOp_BatchTransfers": "197706", - "UserOp_Erc20Transfer": "184680", - "UserOp_InstallSessionKey_Case1": "531042", - "UserOp_NativeTransfer": "160925", + "UserOp_BatchTransfers": "197728", + "UserOp_Erc20Transfer": "184702", + "UserOp_InstallSessionKey_Case1": "530986", + "UserOp_NativeTransfer": "160947", "UserOp_UseSessionKey_Case1_Counter": "194415", "UserOp_UseSessionKey_Case1_Token": "225399", - "UserOp_deferredValidation": "233002" + "UserOp_deferredValidation": "232968" } \ No newline at end of file diff --git a/gas-snapshots/SemiModularAccount.json b/gas-snapshots/SemiModularAccount.json index b6675a60..9ecce98e 100644 --- a/gas-snapshots/SemiModularAccount.json +++ b/gas-snapshots/SemiModularAccount.json @@ -1,16 +1,16 @@ { "Runtime_AccountCreation": "97767", - "Runtime_BatchTransfers": "88758", - "Runtime_Erc20Transfer": "74238", - "Runtime_InstallSessionKey_Case1": "421513", - "Runtime_NativeTransfer": "50397", + "Runtime_BatchTransfers": "88735", + "Runtime_Erc20Transfer": "74215", + "Runtime_InstallSessionKey_Case1": "421412", + "Runtime_NativeTransfer": "50374", "Runtime_UseSessionKey_Case1_Counter": "78834", "Runtime_UseSessionKey_Case1_Token": "112147", - "UserOp_BatchTransfers": "192875", - "UserOp_Erc20Transfer": "179932", - "UserOp_InstallSessionKey_Case1": "528315", - "UserOp_NativeTransfer": "156207", + "UserOp_BatchTransfers": "192852", + "UserOp_Erc20Transfer": "179909", + "UserOp_InstallSessionKey_Case1": "528214", + "UserOp_NativeTransfer": "156184", "UserOp_UseSessionKey_Case1_Counter": "194667", "UserOp_UseSessionKey_Case1_Token": "225651", - "UserOp_deferredValidation": "228953" + "UserOp_deferredValidation": "228829" } \ No newline at end of file diff --git a/src/account/ModularAccount.sol b/src/account/ModularAccount.sol index 580ae5ed..ca55038e 100644 --- a/src/account/ModularAccount.sol +++ b/src/account/ModularAccount.sol @@ -33,4 +33,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 b767822e..4fcfe2f3 100644 --- a/src/account/ModularAccountBase.sol +++ b/src/account/ModularAccountBase.sol @@ -106,8 +106,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, upgradeToAndCall, updateFallbackSignerData. modifier wrapNativeFunction() { DensePostHookData postHookData = _checkPermittedCallerAndAssociatedHooks(); @@ -1075,8 +1075,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 e12429f3..4de9e971 100644 --- a/src/account/ModularAccountView.sol +++ b/src/account/ModularAccountView.sol @@ -2,13 +2,12 @@ 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 {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; @@ -32,16 +31,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; @@ -81,20 +82,26 @@ 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.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.installValidation.selector) + || selector == uint32(IModularAccount.uninstallExecution.selector) + || selector == uint32(IModularAccount.uninstallValidation.selector) + || selector == uint32(IModularAccountBase.performCreate.selector) + || selector == uint32(UUPSUpgradeable.upgradeToAndCall.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 72d71ce5..024e6c51 100644 --- a/src/account/SemiModularAccountBase.sol +++ b/src/account/SemiModularAccountBase.sol @@ -167,10 +167,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(ValidationLookupKey validationFunction) internal view override returns (bool) { if (validationFunction.eq(FALLBACK_VALIDATION_LOOKUP_KEY) || super._isValidationGlobal(validationFunction)) { @@ -221,8 +217,8 @@ 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); } @@ -276,4 +272,10 @@ abstract contract SemiModularAccountBase is ModularAccountBase { } return res; } + + /// @dev Overrides ModularAccountView. + function _isWrappedNativeFunction(uint32 selector) internal pure virtual override returns (bool) { + return + super._isWrappedNativeFunction(selector) || selector == uint32(this.updateFallbackSignerData.selector); + } } diff --git a/src/account/SemiModularAccountStorageOnly.sol b/src/account/SemiModularAccountStorageOnly.sol index 6a2d2dc9..1a7ac9db 100644 --- a/src/account/SemiModularAccountStorageOnly.sol +++ b/src/account/SemiModularAccountStorageOnly.sol @@ -32,4 +32,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 8e25fc72..580a43bf 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..7b29347f 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,175 @@ 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[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); } }