From 714189b27aeb810c53e41996c761522282bf3b4a Mon Sep 17 00:00:00 2001 From: adam Date: Tue, 19 Mar 2024 17:13:51 -0400 Subject: [PATCH 1/3] Experimental signature validation support --- src/account/AccountStorage.sol | 2 ++ src/account/PluginManagerInternals.sol | 23 ++++++++++++++++------- src/account/UpgradeableModularAccount.sol | 11 +++++++++++ src/interfaces/IPlugin.sol | 11 +++++++++++ src/plugins/BasePlugin.sol | 10 ++++++++++ src/plugins/owner/SingleOwnerPlugin.sol | 11 +---------- 6 files changed, 51 insertions(+), 17 deletions(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 3a8eb0d1..3252e229 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -61,6 +61,8 @@ struct AccountStorage { mapping(IPlugin => mapping(address => PermittedExternalCallData)) permittedExternalCalls; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; + // Holds all added signature validators. + EnumerableSet.AddressSet signatureValidators; } function getAccountStorage() pure returns (AccountStorage storage _storage) { diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index f97946c9..d408f598 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -55,19 +55,28 @@ abstract contract PluginManagerInternals is IPluginManager { // Storage update operations function _setExecutionFunction(bytes4 selector, address plugin) internal notNullPlugin(IPlugin(plugin)) { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + // Temporary specification mechanism for signature validators, prior to multi-validation support. + if (selector == IPlugin.isValidSignatureWithSender.selector) { + getAccountStorage().signatureValidators.add(plugin); + } else { + SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (_selectorData.plugin != address(0)) { - revert ExecutionFunctionAlreadySet(selector); - } + if (_selectorData.plugin != address(0)) { + revert ExecutionFunctionAlreadySet(selector); + } - _selectorData.plugin = plugin; + _selectorData.plugin = plugin; + } } function _removeExecutionFunction(bytes4 selector) internal { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + if (selector == IPlugin.isValidSignatureWithSender.selector) { + getAccountStorage().signatureValidators.remove(msg.sender); + } else { + SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - _selectorData.plugin = address(0); + _selectorData.plugin = address(0); + } } function _addValidationFunction(bytes4 selector, IPlugin validation) internal notNullPlugin(validation) { diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 1fb60f13..bf6e19c8 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -6,6 +6,7 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; @@ -28,6 +29,7 @@ contract UpgradeableModularAccount is AccountStorageInitializable, BaseAccount, IERC165, + IERC1271, IPluginExecutor, IStandardExecutor, PluginManagerInternals, @@ -35,6 +37,7 @@ contract UpgradeableModularAccount is { using EnumerableMap for EnumerableMap.Bytes32ToUintMap; using EnumerableSet for EnumerableSet.Bytes32Set; + using EnumerableSet for EnumerableSet.AddressSet; struct PostExecToRun { bytes preExecHookReturnData; @@ -300,6 +303,14 @@ contract UpgradeableModularAccount is return getAccountStorage().supportedIfaces[interfaceId] > 0; } + function isValidSignature(bytes32 hash, bytes calldata signature) external view override returns (bytes4) { + // Uses the first signature validator that is added. + // Multiple validation support will be implemented in a separate PR. + return IPlugin(getAccountStorage().signatureValidators.at(0)).isValidSignatureWithSender( + msg.sender, hash, signature + ); + } + /// @inheritdoc UUPSUpgradeable function upgradeTo(address newImplementation) public override onlyProxy wrapNativeFunction { _upgradeToAndCallUUPS(newImplementation, new bytes(0), false); diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index fff4066a..0185c51b 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -137,6 +137,17 @@ interface IPlugin { /// @param data The calldata sent. function validateRuntime(address sender, uint256 value, bytes calldata data) external; + /// @notice Validates a signature using ERC-1271. + /// @param sender the address that sent the ERC-1271 request to the smart account + /// @param hash the hash of the ERC-1271 request + /// @param signature the signature of the ERC-1271 request + /// + /// @return the ERC-1271 `MAGIC_VALUE` if the signature is valid, or 0xFFFFFFFF if invalid. + function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature) + external + view + returns (bytes4); + /// @notice Run the pre execution hook. /// @dev To indicate the entire call should revert, the function MUST revert. /// @param sender The caller address. diff --git a/src/plugins/BasePlugin.sol b/src/plugins/BasePlugin.sol index ffb15d74..f1744ef6 100644 --- a/src/plugins/BasePlugin.sol +++ b/src/plugins/BasePlugin.sol @@ -57,6 +57,16 @@ abstract contract BasePlugin is ERC165, IPlugin { revert NotImplemented(); } + function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature) + external + pure + override + returns (bytes4) + { + (sender, hash, signature); + revert NotImplemented(); + } + /// @inheritdoc IPlugin function preExecutionHook(address sender, uint256 value, bytes calldata data) external diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index b6427c3f..c177262f 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -139,7 +139,7 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0 // Unused. }); - manifest.validationFunctions = new ManifestAssociatedFunction[](8); + manifest.validationFunctions = new ManifestAssociatedFunction[](7); manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.transferOwnership.selector, associatedFunction: ownerValidationFunction @@ -169,15 +169,6 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { associatedFunction: ownerValidationFunction }); - ManifestFunction memory alwaysAllowFunction = ManifestFunction({ - functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - dependencyIndex: 0 // Unused. - }); - manifest.validationFunctions[7] = ManifestAssociatedFunction({ - executionSelector: this.isValidSignature.selector, - associatedFunction: alwaysAllowFunction - }); - return manifest; } From 0882535de3921389f977d1854e0a397580480589 Mon Sep 17 00:00:00 2001 From: adam Date: Tue, 19 Mar 2024 17:24:18 -0400 Subject: [PATCH 2/3] Update interfaces and tests --- src/plugins/BasePlugin.sol | 5 +++-- src/plugins/owner/SingleOwnerPlugin.sol | 9 ++++----- test/account/UpgradeableModularAccount.t.sol | 8 ++++++++ test/plugin/SingleOwnerPlugin.t.sol | 6 +++--- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/plugins/BasePlugin.sol b/src/plugins/BasePlugin.sol index f1744ef6..f4409be0 100644 --- a/src/plugins/BasePlugin.sol +++ b/src/plugins/BasePlugin.sol @@ -57,10 +57,11 @@ abstract contract BasePlugin is ERC165, IPlugin { revert NotImplemented(); } + /// @inheritdoc IPlugin function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature) external - pure - override + virtual + view returns (bytes4) { (sender, hash, signature); diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index c177262f..42103a01 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.19; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; @@ -36,7 +35,7 @@ import {ISingleOwnerPlugin} from "./ISingleOwnerPlugin.sol"; /// the account, violating storage access rules. This also means that the /// owner of a modular account may not be another modular account if you want to /// send user operations through a bundler. -contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { +contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin { using ECDSA for bytes32; string public constant NAME = "Single Owner Plugin"; @@ -60,14 +59,14 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { _transferOwnership(newOwner); } - /// @inheritdoc IERC1271 /// @dev The signature is valid if it is signed by the owner's private key /// (if the owner is an EOA) or if it is a valid ERC-1271 signature from the /// owner (if the owner is a contract). Note that unlike the signature /// validation used in `validateUserOp`, this does///*not** wrap the digest in /// an "Ethereum Signed Message" envelope before checking the signature in /// the EOA-owner case. - function isValidSignature(bytes32 digest, bytes memory signature) public view override returns (bytes4) { + function isValidSignatureWithSender(address sender, bytes32 digest, bytes memory signature) public view override returns (bytes4) { + (sender); if (SignatureChecker.isValidSignatureNow(_owners[msg.sender], digest, signature)) { return _1271_MAGIC_VALUE; } @@ -132,7 +131,7 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { manifest.executionFunctions = new bytes4[](3); manifest.executionFunctions[0] = this.transferOwnership.selector; - manifest.executionFunctions[1] = this.isValidSignature.selector; + manifest.executionFunctions[1] = this.isValidSignatureWithSender.selector; manifest.executionFunctions[2] = this.owner.selector; ManifestFunction memory ownerValidationFunction = ManifestFunction({ diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 94dd2d0e..d88d6842 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -420,6 +420,14 @@ contract UpgradeableModularAccountTest is OptimizedTest { assertEq(plugins[1], address(plugin)); } + function test_isValidSignature_EOA() public { + bytes32 digest = keccak256("test"); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, digest); + + // sig check should pass using 1271 magic value + assertEq(bytes4(0x1626ba7e), account2.isValidSignature(digest, abi.encodePacked(r, s, v))); + } + function _installPluginWithExecHooks() internal returns (MockPlugin plugin) { vm.startPrank(owner2); diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index cb709fa4..ad9b98bd 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -151,20 +151,20 @@ contract SingleOwnerPluginTest is OptimizedTest { (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, digest); // sig check should fail - assertEq(plugin.isValidSignature(digest, abi.encodePacked(r, s, v)), bytes4(0xFFFFFFFF)); + assertEq(plugin.isValidSignatureWithSender(address(0), digest, abi.encodePacked(r, s, v)), bytes4(0xFFFFFFFF)); // transfer ownership to signer plugin.transferOwnership(signer); assertEq(signer, plugin.owner()); // sig check should pass - assertEq(plugin.isValidSignature(digest, abi.encodePacked(r, s, v)), _1271_MAGIC_VALUE); + assertEq(plugin.isValidSignatureWithSender(address(0), digest, abi.encodePacked(r, s, v)), _1271_MAGIC_VALUE); } function testFuzz_isValidSignatureForContractOwner(bytes32 digest) public { vm.startPrank(a); plugin.transferOwnership(address(contractOwner)); bytes memory signature = contractOwner.sign(digest); - assertEq(plugin.isValidSignature(digest, signature), _1271_MAGIC_VALUE); + assertEq(plugin.isValidSignatureWithSender(address(0), digest, signature), _1271_MAGIC_VALUE); } } From 6c9f6e5a4802c4556ea836f64f6006fd7170a661 Mon Sep 17 00:00:00 2001 From: adam Date: Tue, 19 Mar 2024 17:32:36 -0400 Subject: [PATCH 3/3] lint --- src/interfaces/IPlugin.sol | 22 +++++++++++----------- src/plugins/BasePlugin.sol | 22 +++++++++++----------- src/plugins/owner/SingleOwnerPlugin.sol | 7 ++++++- test/plugin/SingleOwnerPlugin.t.sol | 8 ++++++-- 4 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 0185c51b..ff3e40a9 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -137,17 +137,6 @@ interface IPlugin { /// @param data The calldata sent. function validateRuntime(address sender, uint256 value, bytes calldata data) external; - /// @notice Validates a signature using ERC-1271. - /// @param sender the address that sent the ERC-1271 request to the smart account - /// @param hash the hash of the ERC-1271 request - /// @param signature the signature of the ERC-1271 request - /// - /// @return the ERC-1271 `MAGIC_VALUE` if the signature is valid, or 0xFFFFFFFF if invalid. - function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature) - external - view - returns (bytes4); - /// @notice Run the pre execution hook. /// @dev To indicate the entire call should revert, the function MUST revert. /// @param sender The caller address. @@ -163,6 +152,17 @@ interface IPlugin { /// @param preExecHookData The context returned by its associated pre execution hook. function postExecutionHook(bytes calldata preExecHookData) external; + /// @notice Validates a signature using ERC-1271. + /// @param sender the address that sent the ERC-1271 request to the smart account + /// @param hash the hash of the ERC-1271 request + /// @param signature the signature of the ERC-1271 request + /// + /// @return the ERC-1271 `MAGIC_VALUE` if the signature is valid, or 0xFFFFFFFF if invalid. + function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature) + external + view + returns (bytes4); + /// @notice Describe the contents and intended configuration of the plugin. /// @dev This manifest MUST stay constant over time. /// @return A manifest describing the contents and intended configuration of the plugin. diff --git a/src/plugins/BasePlugin.sol b/src/plugins/BasePlugin.sol index f4409be0..37aedf47 100644 --- a/src/plugins/BasePlugin.sol +++ b/src/plugins/BasePlugin.sol @@ -57,17 +57,6 @@ abstract contract BasePlugin is ERC165, IPlugin { revert NotImplemented(); } - /// @inheritdoc IPlugin - function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature) - external - virtual - view - returns (bytes4) - { - (sender, hash, signature); - revert NotImplemented(); - } - /// @inheritdoc IPlugin function preExecutionHook(address sender, uint256 value, bytes calldata data) external @@ -84,6 +73,17 @@ abstract contract BasePlugin is ERC165, IPlugin { revert NotImplemented(); } + /// @inheritdoc IPlugin + function isValidSignatureWithSender(address sender, bytes32 hash, bytes calldata signature) + external + view + virtual + returns (bytes4) + { + (sender, hash, signature); + revert NotImplemented(); + } + /// @inheritdoc IPlugin function pluginManifest() external pure virtual returns (PluginManifest memory) { revert NotImplemented(); diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index 42103a01..a5c677db 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -65,7 +65,12 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin { /// validation used in `validateUserOp`, this does///*not** wrap the digest in /// an "Ethereum Signed Message" envelope before checking the signature in /// the EOA-owner case. - function isValidSignatureWithSender(address sender, bytes32 digest, bytes memory signature) public view override returns (bytes4) { + function isValidSignatureWithSender(address sender, bytes32 digest, bytes memory signature) + public + view + override + returns (bytes4) + { (sender); if (SignatureChecker.isValidSignatureNow(_owners[msg.sender], digest, signature)) { return _1271_MAGIC_VALUE; diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index ad9b98bd..fa77bcf5 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -151,14 +151,18 @@ contract SingleOwnerPluginTest is OptimizedTest { (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, digest); // sig check should fail - assertEq(plugin.isValidSignatureWithSender(address(0), digest, abi.encodePacked(r, s, v)), bytes4(0xFFFFFFFF)); + assertEq( + plugin.isValidSignatureWithSender(address(0), digest, abi.encodePacked(r, s, v)), bytes4(0xFFFFFFFF) + ); // transfer ownership to signer plugin.transferOwnership(signer); assertEq(signer, plugin.owner()); // sig check should pass - assertEq(plugin.isValidSignatureWithSender(address(0), digest, abi.encodePacked(r, s, v)), _1271_MAGIC_VALUE); + assertEq( + plugin.isValidSignatureWithSender(address(0), digest, abi.encodePacked(r, s, v)), _1271_MAGIC_VALUE + ); } function testFuzz_isValidSignatureForContractOwner(bytes32 digest) public {