From 5b5be8f18730315ee2d8a671656d448ec6572b18 Mon Sep 17 00:00:00 2001 From: Akshay Date: Fri, 27 Oct 2023 14:29:03 +0200 Subject: [PATCH 1/6] Add userop validator handler --- contracts/SafeProtocolManager.sol | 2 +- contracts/base/FunctionHandlerManager.sol | 34 ++++++++++++++++---- deploy/deploy_protocol.ts | 10 +++++- deploy/deploy_protocol_with_test_registry.ts | 9 +++++- hardhat.config.ts | 18 +++++++++-- 5 files changed, 60 insertions(+), 13 deletions(-) diff --git a/contracts/SafeProtocolManager.sol b/contracts/SafeProtocolManager.sol index 7012b720..6a858ddc 100644 --- a/contracts/SafeProtocolManager.sol +++ b/contracts/SafeProtocolManager.sol @@ -19,7 +19,7 @@ import {MODULE_TYPE_PLUGIN} from "./common/Constants.sol"; * plugins through a Manager rather than directly enabling plugins in their Account. * Users have to first enable SafeProtocolManager as a plugin on their Account and then enable other plugins through the manager. */ -contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksManager, FunctionHandlerManager, IERC165 { +contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksManager, IERC165 { address internal constant SENTINEL_MODULES = address(0x1); /** diff --git a/contracts/base/FunctionHandlerManager.sol b/contracts/base/FunctionHandlerManager.sol index 984565a6..15880cd5 100644 --- a/contracts/base/FunctionHandlerManager.sol +++ b/contracts/base/FunctionHandlerManager.sol @@ -12,12 +12,20 @@ import {MODULE_TYPE_FUNCTION_HANDLER} from "../common/Constants.sol"; * @notice This contract manages the function handlers for an Account. The contract stores the * information about an account, bytes4 function selector and the function handler contract address. */ -abstract contract FunctionHandlerManager is RegistryManager { +contract FunctionHandlerManager is RegistryManager { // Storage /** @dev Mapping that stores information about an account, function selector, and address of the account. */ mapping(address => mapping(bytes4 => address)) public functionHandlers; + mapping(address => bool) public isValidateUserOpHandlerEnabled; + address public validateUserOpHandler; + bytes4 constant validateUserOpSelector = bytes4(0x3a871cdd); + + constructor(address _validateUserOpHandler, address registry, address initialOwner) RegistryManager(registry, initialOwner) { + validateUserOpHandler = _validateUserOpHandler; + } + // Events event FunctionHandlerChanged(address indexed account, bytes4 indexed selector, address indexed functionHandler); @@ -52,6 +60,14 @@ abstract contract FunctionHandlerManager is RegistryManager { emit FunctionHandlerChanged(msg.sender, selector, functionHandler); } + function setValidateUserOpHandler(address newHandler) external onlyOwner { + validateUserOpHandler = newHandler; + } + + function enableUserOpValidator() external onlyAccount { + isValidateUserOpHandlerEnabled[msg.sender] = true; + } + /** * @notice This fallback handler function checks if an account (msg.sender) has a function handler enabled. * If enabled, calls handle function and returns the result back. @@ -63,6 +79,16 @@ abstract contract FunctionHandlerManager is RegistryManager { address account = msg.sender; bytes4 functionSelector = bytes4(msg.data); + address sender; + // solhint-disable-next-line no-inline-assembly + assembly { + sender := shr(96, calldataload(sub(calldatasize(), 20))) + } + + if (validateUserOpSelector == functionSelector && isValidateUserOpHandlerEnabled[account]) { + return ISafeProtocolFunctionHandler(validateUserOpHandler).handle(account, sender, 0, msg.data); + } + address functionHandler = functionHandlers[account][functionSelector]; // Revert if functionHandler is not set @@ -70,12 +96,6 @@ abstract contract FunctionHandlerManager is RegistryManager { revert FunctionHandlerNotSet(account, functionSelector); } - address sender; - // solhint-disable-next-line no-inline-assembly - assembly { - sender := shr(96, calldataload(sub(calldatasize(), 20))) - } - // With a Safe{Core} Account v1.x, msg.data contains 20 bytes of sender address. Read the sender address by loading last 20 bytes. // remove last 20 bytes from calldata and store it in `data`. // Keep first 4 bytes (i.e function signature) so that handler contract can infer function identifier. diff --git a/deploy/deploy_protocol.ts b/deploy/deploy_protocol.ts index 39e1187d..94e7ba33 100644 --- a/deploy/deploy_protocol.ts +++ b/deploy/deploy_protocol.ts @@ -3,7 +3,7 @@ import { HardhatRuntimeEnvironment } from "hardhat/types"; const deploy: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { const { deployments, getNamedAccounts } = hre; - const { deployer, owner } = await getNamedAccounts(); + const { deployer, owner, userOpValidatorHandler } = await getNamedAccounts(); const { deploy } = deployments; const registry = await deploy("SafeProtocolRegistry", { from: deployer, @@ -18,6 +18,14 @@ const deploy: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { log: true, deterministicDeployment: true, }); + + + await deploy("FunctionHandlerManager", { + from: deployer, + args: [userOpValidatorHandler, registry.address, owner], + log: true, + deterministicDeployment: true, + }); }; deploy.tags = ["protocol"]; diff --git a/deploy/deploy_protocol_with_test_registry.ts b/deploy/deploy_protocol_with_test_registry.ts index 3fbb00bd..6536f3a2 100644 --- a/deploy/deploy_protocol_with_test_registry.ts +++ b/deploy/deploy_protocol_with_test_registry.ts @@ -3,7 +3,7 @@ import { HardhatRuntimeEnvironment } from "hardhat/types"; const deploy: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { const { deployments, getNamedAccounts } = hre; - const { deployer, owner } = await getNamedAccounts(); + const { deployer, owner, userOpValidatorHandler } = await getNamedAccounts(); const { deploy } = deployments; const testRegistry = await deploy("TestSafeProtocolRegistryUnrestricted", { from: deployer, @@ -18,6 +18,13 @@ const deploy: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { log: true, deterministicDeployment: true, }); + + await deploy("FunctionHandlerManager", { + from: deployer, + args: [userOpValidatorHandler, testRegistry.address, owner], + log: true, + deterministicDeployment: true, + }); }; deploy.tags = ["test-protocol"]; diff --git a/hardhat.config.ts b/hardhat.config.ts index 75913490..2ee5676e 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -7,7 +7,7 @@ import { HttpNetworkUserConfig } from "hardhat/types"; import "hardhat-deploy"; import { DeterministicDeploymentInfo } from "hardhat-deploy/dist/types"; import { getSingletonFactoryInfo } from "@safe-global/safe-singleton-factory"; -import { ethers } from "ethers"; +import { ZeroAddress, ethers } from "ethers"; import "./src/tasks/generate_deployments_markdown"; import "./src/tasks/show_codesize"; @@ -22,7 +22,7 @@ const argv : any = yargs .help(false) .version(false).argv; -const { NODE_URL, MNEMONIC, INFURA_KEY, ETHERSCAN_API_KEY, SAFE_CORE_PROTOCOL_OWNER_ADDRESS } = process.env; +const { NODE_URL, MNEMONIC, INFURA_KEY, ETHERSCAN_API_KEY, SAFE_CORE_PROTOCOL_OWNER_ADDRESS, SAFE_CORE_PROTOCOL_4337_USER_OP_VALIDATOR_HANDLER_ADDRESS } = process.env; const deterministicDeployment = (network: string): DeterministicDeploymentInfo => { const info = getSingletonFactoryInfo(parseInt(network)); @@ -52,10 +52,19 @@ sharedNetworkConfig.accounts = { } const config: HardhatUserConfig = { - solidity: "0.8.18", + solidity: { + version: "0.8.18", + settings: { + optimizer: { + enabled: true, + runs: 2000 + }, + } + }, gasReporter: { enabled: (process.env.REPORT_GAS) ? true : false }, + networks: { hardhat: { allowUnlimitedContractSize: true, @@ -109,6 +118,9 @@ const config: HardhatUserConfig = { }, owner: { default: SAFE_CORE_PROTOCOL_OWNER_ADDRESS || 1 + }, + userOpValidatorHandler: { + default: SAFE_CORE_PROTOCOL_4337_USER_OP_VALIDATOR_HANDLER_ADDRESS || ZeroAddress } } }; From 9581f96d73230d2cb105e4c0a658b1f980c37a2b Mon Sep 17 00:00:00 2001 From: Akshay Date: Fri, 27 Oct 2023 15:15:29 +0200 Subject: [PATCH 2/6] Use mapping of validator address --- contracts/base/FunctionHandlerManager.sol | 24 ++++++++++------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/contracts/base/FunctionHandlerManager.sol b/contracts/base/FunctionHandlerManager.sol index 15880cd5..1884cafc 100644 --- a/contracts/base/FunctionHandlerManager.sol +++ b/contracts/base/FunctionHandlerManager.sol @@ -18,12 +18,10 @@ contract FunctionHandlerManager is RegistryManager { */ mapping(address => mapping(bytes4 => address)) public functionHandlers; - mapping(address => bool) public isValidateUserOpHandlerEnabled; - address public validateUserOpHandler; + mapping(address => address) public validateUserOpHandler; bytes4 constant validateUserOpSelector = bytes4(0x3a871cdd); - constructor(address _validateUserOpHandler, address registry, address initialOwner) RegistryManager(registry, initialOwner) { - validateUserOpHandler = _validateUserOpHandler; + constructor(address registry, address initialOwner) RegistryManager(registry, initialOwner) { } // Events @@ -49,6 +47,12 @@ contract FunctionHandlerManager is RegistryManager { * @param functionHandler Address of the contract to be set as a function handler */ function setFunctionHandler(bytes4 selector, address functionHandler) external onlyAccount { + + if (selector == validateUserOpSelector) { + validateUserOpHandler[msg.sender] = functionHandler; + emit FunctionHandlerChanged(msg.sender, selector, functionHandler); + return; + } if (functionHandler != address(0)) { checkPermittedModule(functionHandler, MODULE_TYPE_FUNCTION_HANDLER); if (!ISafeProtocolFunctionHandler(functionHandler).supportsInterface(type(ISafeProtocolFunctionHandler).interfaceId)) @@ -60,14 +64,6 @@ contract FunctionHandlerManager is RegistryManager { emit FunctionHandlerChanged(msg.sender, selector, functionHandler); } - function setValidateUserOpHandler(address newHandler) external onlyOwner { - validateUserOpHandler = newHandler; - } - - function enableUserOpValidator() external onlyAccount { - isValidateUserOpHandlerEnabled[msg.sender] = true; - } - /** * @notice This fallback handler function checks if an account (msg.sender) has a function handler enabled. * If enabled, calls handle function and returns the result back. @@ -85,8 +81,8 @@ contract FunctionHandlerManager is RegistryManager { sender := shr(96, calldataload(sub(calldatasize(), 20))) } - if (validateUserOpSelector == functionSelector && isValidateUserOpHandlerEnabled[account]) { - return ISafeProtocolFunctionHandler(validateUserOpHandler).handle(account, sender, 0, msg.data); + if (validateUserOpSelector == functionSelector && validateUserOpHandler[account] != address(0)) { + return ISafeProtocolFunctionHandler(validateUserOpHandler[account]).handle(account, sender, 0, msg.data); } address functionHandler = functionHandlers[account][functionSelector]; From 415c68d690de61ee8e8bfccf3f73ba72cc21f9f8 Mon Sep 17 00:00:00 2001 From: Akshay Date: Fri, 27 Oct 2023 15:36:31 +0200 Subject: [PATCH 3/6] WIP: function handler for validation userOp --- contracts/base/FunctionHandlerManager.sol | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/contracts/base/FunctionHandlerManager.sol b/contracts/base/FunctionHandlerManager.sol index 1884cafc..a146a71e 100644 --- a/contracts/base/FunctionHandlerManager.sol +++ b/contracts/base/FunctionHandlerManager.sol @@ -12,7 +12,7 @@ import {MODULE_TYPE_FUNCTION_HANDLER} from "../common/Constants.sol"; * @notice This contract manages the function handlers for an Account. The contract stores the * information about an account, bytes4 function selector and the function handler contract address. */ -contract FunctionHandlerManager is RegistryManager { +abstract contract FunctionHandlerManager is RegistryManager { // Storage /** @dev Mapping that stores information about an account, function selector, and address of the account. */ @@ -21,9 +21,6 @@ contract FunctionHandlerManager is RegistryManager { mapping(address => address) public validateUserOpHandler; bytes4 constant validateUserOpSelector = bytes4(0x3a871cdd); - constructor(address registry, address initialOwner) RegistryManager(registry, initialOwner) { - } - // Events event FunctionHandlerChanged(address indexed account, bytes4 indexed selector, address indexed functionHandler); @@ -47,7 +44,6 @@ contract FunctionHandlerManager is RegistryManager { * @param functionHandler Address of the contract to be set as a function handler */ function setFunctionHandler(bytes4 selector, address functionHandler) external onlyAccount { - if (selector == validateUserOpSelector) { validateUserOpHandler[msg.sender] = functionHandler; emit FunctionHandlerChanged(msg.sender, selector, functionHandler); From 1f16492b74ac35fb17ff38e992513d21d7fdcc17 Mon Sep 17 00:00:00 2001 From: Akshay Date: Fri, 27 Oct 2023 16:22:58 +0200 Subject: [PATCH 4/6] WIP: 4337 fix --- contracts/base/FunctionHandlerManager.sol | 4 +++- deploy/deploy_protocol.ts | 3 +-- deploy/deploy_protocol_with_test_registry.ts | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/contracts/base/FunctionHandlerManager.sol b/contracts/base/FunctionHandlerManager.sol index a146a71e..852c15e7 100644 --- a/contracts/base/FunctionHandlerManager.sol +++ b/contracts/base/FunctionHandlerManager.sol @@ -12,7 +12,7 @@ import {MODULE_TYPE_FUNCTION_HANDLER} from "../common/Constants.sol"; * @notice This contract manages the function handlers for an Account. The contract stores the * information about an account, bytes4 function selector and the function handler contract address. */ -abstract contract FunctionHandlerManager is RegistryManager { +contract FunctionHandlerManager is RegistryManager { // Storage /** @dev Mapping that stores information about an account, function selector, and address of the account. */ @@ -27,6 +27,8 @@ abstract contract FunctionHandlerManager is RegistryManager { // Errors error FunctionHandlerNotSet(address account, bytes4 functionSelector); + constructor(address registryAddress, address _initialOwner) RegistryManager(registryAddress, _initialOwner) {} + /** * @notice Returns the function handler for an account and function selector. * @param account Address of an account diff --git a/deploy/deploy_protocol.ts b/deploy/deploy_protocol.ts index 94e7ba33..4c363ef0 100644 --- a/deploy/deploy_protocol.ts +++ b/deploy/deploy_protocol.ts @@ -19,10 +19,9 @@ const deploy: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { deterministicDeployment: true, }); - await deploy("FunctionHandlerManager", { from: deployer, - args: [userOpValidatorHandler, registry.address, owner], + args: [registry.address, owner], log: true, deterministicDeployment: true, }); diff --git a/deploy/deploy_protocol_with_test_registry.ts b/deploy/deploy_protocol_with_test_registry.ts index 6536f3a2..0de02f93 100644 --- a/deploy/deploy_protocol_with_test_registry.ts +++ b/deploy/deploy_protocol_with_test_registry.ts @@ -21,7 +21,7 @@ const deploy: DeployFunction = async function (hre: HardhatRuntimeEnvironment) { await deploy("FunctionHandlerManager", { from: deployer, - args: [userOpValidatorHandler, testRegistry.address, owner], + args: [testRegistry.address, owner], log: true, deterministicDeployment: true, }); From 119aca10047e8899de951d7f90a0178390263c7b Mon Sep 17 00:00:00 2001 From: Akshay Date: Fri, 27 Oct 2023 22:30:18 +0200 Subject: [PATCH 5/6] WIP: 4337 --- contracts/SafeProtocolManager.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/SafeProtocolManager.sol b/contracts/SafeProtocolManager.sol index 6a858ddc..150d4dbe 100644 --- a/contracts/SafeProtocolManager.sol +++ b/contracts/SafeProtocolManager.sol @@ -174,7 +174,7 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana function enablePlugin( address plugin, uint8 permissions - ) external noZeroOrSentinelPlugin(plugin) onlyPermittedModule(plugin, MODULE_TYPE_PLUGIN) onlyAccount { + ) external noZeroOrSentinelPlugin(plugin) onlyPermittedModule(plugin, MODULE_TYPE_PLUGIN) { // address(0) check omitted because it is not expected to enable it as a plugin and // call to it would fail. Additionally, registry should not permit address(0) as an module. if (!ISafeProtocolPlugin(plugin).supportsInterface(type(ISafeProtocolPlugin).interfaceId)) @@ -207,7 +207,7 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana * @notice Disable a plugin. This function should be called by account. * @param plugin Plugin to be disabled */ - function disablePlugin(address prevPlugin, address plugin) external noZeroOrSentinelPlugin(plugin) onlyAccount { + function disablePlugin(address prevPlugin, address plugin) external noZeroOrSentinelPlugin(plugin) { PluginAccessInfo storage prevPluginInfo = enabledPlugins[msg.sender][prevPlugin]; PluginAccessInfo storage pluginInfo = enabledPlugins[msg.sender][plugin]; From 465b85cbafe3685eba2664e776e1b8de3001b3b7 Mon Sep 17 00:00:00 2001 From: Akshay Date: Tue, 31 Oct 2023 15:37:51 +0100 Subject: [PATCH 6/6] wip --- contracts/SafeProtocolManager.sol | 35 +++++++++++++++++-------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/contracts/SafeProtocolManager.sol b/contracts/SafeProtocolManager.sol index 150d4dbe..7f830540 100644 --- a/contracts/SafeProtocolManager.sol +++ b/contracts/SafeProtocolManager.sol @@ -76,16 +76,16 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana */ function executeTransaction( address account, - SafeTransaction calldata transaction - ) external override onlyEnabledPlugin(account) onlyPermittedModule(msg.sender, MODULE_TYPE_PLUGIN) returns (bytes[] memory data) { - address hooksAddress = enabledHooks[account]; - bool areHooksEnabled = hooksAddress != address(0); - bytes memory preCheckData; - if (areHooksEnabled) { - // execution metadata for transaction execution through plugin is encoded address of the plugin i.e. msg.sender. - // executionType = 1 for plugin flow - preCheckData = ISafeProtocolHooks(hooksAddress).preCheck(account, transaction, 1, abi.encode(msg.sender)); - } + SafeTransaction calldata transaction // onlyEnabledPlugin(account) onlyPermittedModule(msg.sender, MODULE_TYPE_PLUGIN) + ) external override returns (bytes[] memory data) { + // address hooksAddress = enabledHooks[account]; + // bool areHooksEnabled = hooksAddress != address(0); + // bytes memory preCheckData; + // if (areHooksEnabled) { + // // execution metadata for transaction execution through plugin is encoded address of the plugin i.e. msg.sender. + // // executionType = 1 for plugin flow + // preCheckData = ISafeProtocolHooks(hooksAddress).preCheck(account, transaction, 1, abi.encode(msg.sender)); + // } data = new bytes[](transaction.actions.length); uint256 length = transaction.actions.length; @@ -97,7 +97,7 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana } else if (safeProtocolAction.to == account) { checkPermission(account, PLUGIN_PERMISSION_CALL_TO_SELF); } else { - checkPermission(account, PLUGIN_PERMISSION_EXECUTE_CALL); + // checkPermission(account, PLUGIN_PERMISSION_EXECUTE_CALL); } (bool isActionSuccessful, bytes memory resultData) = IAccount(account).execTransactionFromModuleReturnData( @@ -114,10 +114,10 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana data[i] = resultData; } } - if (areHooksEnabled) { - // success = true because if transaction is not revereted till here, all actions executed successfully. - ISafeProtocolHooks(hooksAddress).postCheck(account, true, preCheckData); - } + // if (areHooksEnabled) { + // // success = true because if transaction is not revereted till here, all actions executed successfully. + // ISafeProtocolHooks(hooksAddress).postCheck(account, true, preCheckData); + // } emit ActionsExecuted(account, transaction.metadataHash, transaction.nonce); } @@ -174,7 +174,10 @@ contract SafeProtocolManager is ISafeProtocolManager, RegistryManager, HooksMana function enablePlugin( address plugin, uint8 permissions - ) external noZeroOrSentinelPlugin(plugin) onlyPermittedModule(plugin, MODULE_TYPE_PLUGIN) { + ) + external + noZeroOrSentinelPlugin(plugin) // onlyPermittedModule(plugin, MODULE_TYPE_PLUGIN) + { // address(0) check omitted because it is not expected to enable it as a plugin and // call to it would fail. Additionally, registry should not permit address(0) as an module. if (!ISafeProtocolPlugin(plugin).supportsInterface(type(ISafeProtocolPlugin).interfaceId))