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

…tions that don't execute msg.sig-based hooks
alchemyplatform · Nov 22, 2024 · 1956199 · 1956199
1 parent ed5c3e8
commit 1956199
Show file tree

Hide file tree

Showing 9 changed files with 244 additions and 62 deletions.
diff --git a/gas-snapshots/ModularAccount.json b/gas-snapshots/ModularAccount.json
@@ -1,16 +1,16 @@
 {
   "Runtime_AccountCreation": "176147",
-  "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"
 }
diff --git a/gas-snapshots/SemiModularAccount.json b/gas-snapshots/SemiModularAccount.json
@@ -1,16 +1,16 @@
 {
   "Runtime_AccountCreation": "97767",
-  "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"
 }
diff --git 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
@@ -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
@@ -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";
@@ -31,16 +30,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 +80,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.installValidation.selector)
+                || selector == uint32(IModularAccount.uninstallExecution.selector)
+                || selector == uint32(IModularAccount.uninstallValidation.selector)
+                || selector == uint32(IModularAccountBase.invalidateDeferredValidationInstallNonce.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
@@ -143,10 +143,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;
@@ -197,8 +193,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);
     }
@@ -246,4 +242,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
@@ -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
@@ -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)