Skip to content

Commit

Permalink
fix: [S7] don't return exec hooks in getExecutionData for native func…
Browse files Browse the repository at this point in the history
…tions that don't execute msg.sig-based hooks
  • Loading branch information
jaypaik committed Nov 25, 2024
1 parent 74bd948 commit 5fe65eb
Show file tree
Hide file tree
Showing 9 changed files with 246 additions and 63 deletions.
18 changes: 9 additions & 9 deletions gas-snapshots/ModularAccount.json
Original file line number Diff line number Diff line change
@@ -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"
}
18 changes: 9 additions & 9 deletions gas-snapshots/SemiModularAccount.json
Original file line number Diff line number Diff line change
@@ -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"
}
5 changes: 5 additions & 0 deletions src/account/ModularAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
8 changes: 4 additions & 4 deletions src/account/ModularAccountBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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;
}

Expand Down
61 changes: 35 additions & 26 deletions src/account/ModularAccountView.sol
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -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;
Expand Down Expand Up @@ -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)
);
}
}
14 changes: 8 additions & 6 deletions src/account/SemiModularAccountBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
}
5 changes: 5 additions & 0 deletions src/account/SemiModularAccountStorageOnly.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
7 changes: 4 additions & 3 deletions src/helpers/NativeFunctionDelegate.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
Loading

0 comments on commit 5fe65eb

Please sign in to comment.