fix: [S7] don't return exec hooks in getExecutionData for native func…

…tions that don't execute msg.sig-based hooks (#300) Address potential inconsistencies in `ModularAccountView.getExecutionData` if a module with execution hooks on native functions that DO NOT use the `wrapNativeFunction` modifier is installed. Native functions without `wrapNativeFunction` do not run `msg.sig`-based execution hooks. Since we allow installations of modules that clash with native function selectors, it is probably best to give a consistent view of what will actually be executed via our ModularAccountView functions. This PR also includes some refactoring to simplify things, as well as addresses some other consistency issues. 1. Adds `initializeWithValidation` to the native function checks for `ModularAccount`. 1. Adds `initialize` to the native function checks for `SemiModularAccountStorageOnly`. 1. Adds `entryPoint()` as a native function check. 1. Update comment on `wrapNativeFunction` to include all native functions that have the modifier applied within `ModularAccountBase`. 1. Removes unnecessary overriding of `_globalValidationAllowed` within `SemiModularAccountBase`. `_isGlobalValidationAllowedNativeFunction` which is overridden there handles it. 1. Updated `_isGlobalValidationAllowedNativeFunction` to take uint32 instead of bytes4 for consistency and codesize.
alchemyplatform · Nov 25, 2024 · 3ba9547 · 3ba9547
1 parent 95c8b99
commit 3ba9547
Show file tree

Hide file tree

Showing 9 changed files with 246 additions and 63 deletions.
diff --git 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": "429789",
-  "Runtime_NativeTransfer": "54467",
+  "Runtime_BatchTransfers": "92908",
+  "Runtime_Erc20Transfer": "78340",
+  "Runtime_InstallSessionKey_Case1": "429733",
+  "Runtime_NativeTransfer": "54489",
   "Runtime_UseSessionKey_Case1_Counter": "78531",
   "Runtime_UseSessionKey_Case1_Token": "111844",
-  "UserOp_BatchTransfers": "197706",
-  "UserOp_Erc20Transfer": "184680",
-  "UserOp_InstallSessionKey_Case1": "537469",
-  "UserOp_NativeTransfer": "160925",
+  "UserOp_BatchTransfers": "197728",
+  "UserOp_Erc20Transfer": "184702",
+  "UserOp_InstallSessionKey_Case1": "537413",
+  "UserOp_NativeTransfer": "160947",
   "UserOp_UseSessionKey_Case1_Counter": "194415",
   "UserOp_UseSessionKey_Case1_Token": "225399",
-  "UserOp_deferredValidation": "233002"
+  "UserOp_deferredValidation": "232968"
 }
diff --git 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": "427940",
-  "Runtime_NativeTransfer": "50397",
+  "Runtime_BatchTransfers": "88735",
+  "Runtime_Erc20Transfer": "74215",
+  "Runtime_InstallSessionKey_Case1": "427839",
+  "Runtime_NativeTransfer": "50374",
   "Runtime_UseSessionKey_Case1_Counter": "78834",
   "Runtime_UseSessionKey_Case1_Token": "112147",
-  "UserOp_BatchTransfers": "192875",
-  "UserOp_Erc20Transfer": "179932",
-  "UserOp_InstallSessionKey_Case1": "534742",
-  "UserOp_NativeTransfer": "156207",
+  "UserOp_BatchTransfers": "192852",
+  "UserOp_Erc20Transfer": "179909",
+  "UserOp_InstallSessionKey_Case1": "534641",
+  "UserOp_NativeTransfer": "156184",
   "UserOp_UseSessionKey_Case1_Counter": "194667",
   "UserOp_UseSessionKey_Case1_Token": "225651",
-  "UserOp_deferredValidation": "228953"
+  "UserOp_deferredValidation": "228829"
 }
diff --git 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
@@ -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
@@ -1,15 +1,16 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.26;
 
-import {HookConfig, ModuleEntity} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol";
+import {
+    HookConfig,
+    IModularAccount,
+    ModuleEntity
+} 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 +33,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 +84,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
@@ -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
@@ -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
@@ -3,14 +3,14 @@ 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";
 import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol";
 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)