diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 5562fdcd..09a4a974 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -63,8 +63,14 @@ struct AccountStorage { mapping(IPlugin => mapping(address => PermittedExternalCallData)) permittedExternalCalls; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; + // Installed plugins capable of signature validation. + EnumerableSet.Bytes32Set signatureValidations; } +// TODO: Change how pre-validation hooks work to allow association with validation, rather than selector. +// Pre signature validation hooks +// mapping(FunctionReference => EnumerableSet.Bytes32Set) preSignatureValidationHooks; + function getAccountStorage() pure returns (AccountStorage storage _storage) { assembly ("memory-safe") { _storage.slot := _ACCOUNT_STORAGE_SLOT diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 8f228932..a3a108c1 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -262,6 +262,13 @@ abstract contract PluginManagerInternals is IPluginManager { ); } + length = manifest.signatureValidationFunctions.length; + for (uint256 i = 0; i < length; ++i) { + FunctionReference signatureValidationFunction = + FunctionReferenceLib.pack(plugin, manifest.signatureValidationFunctions[i]); + _storage.signatureValidations.add(toSetValue(signatureValidationFunction)); + } + // Hooks are not allowed to be provided as dependencies, so we use an empty array for resolving them. FunctionReference[] memory emptyDependencies; @@ -359,6 +366,13 @@ abstract contract PluginManagerInternals is IPluginManager { ); } + length = manifest.signatureValidationFunctions.length; + for (uint256 i = 0; i < length; ++i) { + FunctionReference signatureValidationFunction = + FunctionReferenceLib.pack(plugin, manifest.signatureValidationFunctions[i]); + _storage.signatureValidations.remove(toSetValue(signatureValidationFunction)); + } + length = manifest.validationFunctions.length; for (uint256 i = 0; i < length; ++i) { ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 96c33ce4..b815e42f 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 {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.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 {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; @@ -24,6 +25,7 @@ import { getAccountStorage, getPermittedCallKey, SelectorData, + toSetValue, toFunctionReference, toExecutionHook } from "./AccountStorage.sol"; @@ -36,6 +38,7 @@ contract UpgradeableModularAccount is AccountStorageInitializable, BaseAccount, IERC165, + IERC1271, IPluginExecutor, IStandardExecutor, PluginManagerInternals, @@ -55,6 +58,10 @@ contract UpgradeableModularAccount is bytes4 internal constant _INTERFACE_ID_INVALID = 0xffffffff; bytes4 internal constant _IERC165_INTERFACE_ID = 0x01ffc9a7; + // bytes4(keccak256("isValidSignature(bytes32,bytes)")) + bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; + bytes4 internal constant _1271_INVALID = 0xffffffff; + event ModularAccountInitialized(IEntryPoint indexed entryPoint); error AlwaysDenyRule(); @@ -67,6 +74,7 @@ contract UpgradeableModularAccount is error PreRuntimeValidationHookFailed(address plugin, uint8 functionId, bytes revertReason); error RuntimeValidationFunctionMissing(bytes4 selector); error RuntimeValidationFunctionReverted(address plugin, uint8 functionId, bytes revertReason); + error SignatureValidationInvalid(address plugin, uint8 functionId); error UnexpectedAggregator(address plugin, uint8 functionId, address aggregator); error UnrecognizedFunction(bytes4 selector); error UserOpValidationFunctionMissing(bytes4 selector); @@ -310,6 +318,25 @@ contract UpgradeableModularAccount is super.upgradeToAndCall(newImplementation, data); } + function isValidSignature(bytes32 hash, bytes calldata signature) public view override returns (bytes4) { + AccountStorage storage _storage = getAccountStorage(); + + FunctionReference sigValidation = FunctionReference.wrap(bytes21(signature)); + + (address plugin, uint8 functionId) = sigValidation.unpack(); + if (!_storage.signatureValidations.contains(toSetValue(sigValidation))) { + revert SignatureValidationInvalid(plugin, functionId); + } + + if ( + IValidation(plugin).validateSignature(functionId, msg.sender, hash, signature[21:]) + == _1271_MAGIC_VALUE + ) { + return _1271_MAGIC_VALUE; + } + return _1271_INVALID; + } + /// @notice Gets the entry point for this account /// @return entryPoint The entry point for this account function entryPoint() public view override returns (IEntryPoint) { diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index cf28c5d2..a229f51b 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -94,6 +94,7 @@ struct PluginManifest { ManifestAssociatedFunction[] validationFunctions; ManifestAssociatedFunction[] preValidationHooks; ManifestExecutionHook[] executionHooks; + uint8[] signatureValidationFunctions; } interface IPlugin is IERC165 { diff --git a/src/interfaces/IValidation.sol b/src/interfaces/IValidation.sol index ac9c68e7..b4edddcc 100644 --- a/src/interfaces/IValidation.sol +++ b/src/interfaces/IValidation.sol @@ -24,4 +24,17 @@ interface IValidation is IPlugin { /// @param value The call value. /// @param data The calldata sent. function validateRuntime(uint8 functionId, address sender, uint256 value, bytes calldata data) external; + + /// @notice Validates a signature using ERC-1271. + /// @dev To indicate the entire call should revert, the function MUST revert. + /// @param functionId An identifier that routes the call to different internal implementations, should there be + /// more than one. + /// @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 validateSignature(uint8 functionId, address sender, bytes32 hash, bytes calldata signature) + external + view + returns (bytes4); } diff --git a/src/interfaces/IValidationHook.sol b/src/interfaces/IValidationHook.sol index 4cbee5e7..8eb7a61d 100644 --- a/src/interfaces/IValidationHook.sol +++ b/src/interfaces/IValidationHook.sol @@ -26,4 +26,19 @@ interface IValidationHook is IPlugin { /// @param data The calldata sent. function preRuntimeValidationHook(uint8 functionId, address sender, uint256 value, bytes calldata data) external; + + // TODO: support this hook type within the account & in the manifest + + /// @notice Run the pre signature validation hook specified by the `functionId`. + /// @dev To indicate the call should revert, the function MUST revert. + /// @param functionId An identifier that routes the call to different internal implementations, should there be + /// more than one. + /// @param sender The caller address. + /// @param hash The hash of the message being signed. + /// @param signature The signature of the message. + // function preSignatureValidationHook(uint8 functionId, address sender, bytes32 hash, bytes calldata + // signature) + // external + // view + // returns (bytes4); } diff --git a/src/plugins/owner/ISingleOwnerPlugin.sol b/src/plugins/owner/ISingleOwnerPlugin.sol index 0c3d8255..6d80eb50 100644 --- a/src/plugins/owner/ISingleOwnerPlugin.sol +++ b/src/plugins/owner/ISingleOwnerPlugin.sol @@ -5,7 +5,8 @@ import {IValidation} from "../../interfaces/IValidation.sol"; interface ISingleOwnerPlugin is IValidation { enum FunctionId { - VALIDATION_OWNER_OR_SELF + VALIDATION_OWNER_OR_SELF, + SIG_VALIDATION } /// @notice This event is emitted when ownership of the account changes. diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index dc5bae13..825b0230 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.25; 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 {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; @@ -39,7 +38,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 ISingleOwnerPlugin, BasePlugin, IERC1271 { +contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { using ECDSA for bytes32; using MessageHashUtils for bytes32; @@ -52,6 +51,7 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin, IERC1271 { // bytes4(keccak256("isValidSignature(bytes32,bytes)")) bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; + bytes4 internal constant _1271_INVALID = 0xffffffff; mapping(address => address) internal _owners; @@ -112,18 +112,26 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin, IERC1271 { // ┃ Execution view functions ┃ // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - /// @inheritdoc IERC1271 + /// @inheritdoc IValidation /// @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) external view override returns (bytes4) { - if (SignatureChecker.isValidSignatureNow(_owners[msg.sender], digest, signature)) { - return _1271_MAGIC_VALUE; + function validateSignature(uint8 functionId, address, bytes32 digest, bytes calldata signature) + external + view + override + returns (bytes4) + { + if (functionId == uint8(FunctionId.SIG_VALIDATION)) { + if (SignatureChecker.isValidSignatureNow(_owners[msg.sender], digest, signature)) { + return _1271_MAGIC_VALUE; + } + return _1271_INVALID; } - return 0xffffffff; + revert NotImplemented(); } /// @inheritdoc ISingleOwnerPlugin @@ -144,17 +152,16 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin, IERC1271 { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new bytes4[](3); + manifest.executionFunctions = new bytes4[](2); manifest.executionFunctions[0] = this.transferOwnership.selector; - manifest.executionFunctions[1] = this.isValidSignature.selector; - manifest.executionFunctions[2] = this.owner.selector; + manifest.executionFunctions[1] = this.owner.selector; ManifestFunction memory ownerValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, functionId: uint8(FunctionId.VALIDATION_OWNER_OR_SELF), dependencyIndex: 0 // Unused. }); - manifest.validationFunctions = new ManifestAssociatedFunction[](8); + manifest.validationFunctions = new ManifestAssociatedFunction[](7); manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.transferOwnership.selector, associatedFunction: ownerValidationFunction @@ -186,14 +193,13 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin, IERC1271 { dependencyIndex: 0 // Unused. }); manifest.validationFunctions[6] = ManifestAssociatedFunction({ - executionSelector: this.isValidSignature.selector, - associatedFunction: alwaysAllowRuntime - }); - manifest.validationFunctions[7] = ManifestAssociatedFunction({ executionSelector: this.owner.selector, associatedFunction: alwaysAllowRuntime }); + manifest.signatureValidationFunctions = new uint8[](1); + manifest.signatureValidationFunctions[0] = uint8(FunctionId.SIG_VALIDATION); + return manifest; } diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index c1c0580c..363d1654 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -6,6 +6,7 @@ import {console} from "forge-std/Test.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; +import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; @@ -16,6 +17,7 @@ import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {Call} from "../../src/interfaces/IStandardExecutor.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; +import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; import {Counter} from "../mocks/Counter.sol"; import {ComprehensivePlugin} from "../mocks/plugins/ComprehensivePlugin.sol"; @@ -428,6 +430,22 @@ contract UpgradeableModularAccountTest is AccountTestBase { assertEq(SingleOwnerPlugin(address(account1)).owner(), owner2); } + function test_isValidSignature() public { + bytes32 message = keccak256("hello world"); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, message); + + // singleOwnerPlugin.ownerOf(address(account1)); + + bytes memory signature = abi.encodePacked( + address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.SIG_VALIDATION), r, s, v + ); + + bytes4 validationResult = IERC1271(address(account1)).isValidSignature(message, signature); + + assertEq(validationResult, bytes4(0x1626ba7e)); + } + // Internal Functions function _printStorageReadsAndWrites(address addr) internal { diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index edc8379e..bdb82499 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -26,7 +26,8 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba VALIDATION, BOTH_EXECUTION_HOOKS, PRE_EXECUTION_HOOK, - POST_EXECUTION_HOOK + POST_EXECUTION_HOOK, + SIG_VALIDATION } string public constant NAME = "Comprehensive Plugin"; @@ -89,6 +90,17 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } + function validateSignature(uint8 functionId, address, bytes32, bytes calldata) + external + pure + returns (bytes4) + { + if (functionId == uint8(FunctionId.SIG_VALIDATION)) { + return 0xffffffff; + } + revert NotImplemented(); + } + function preExecutionHook(uint8 functionId, address, uint256, bytes calldata) external pure diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index 48ca0ea1..9e7909df 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -59,6 +59,10 @@ abstract contract MockBaseUserOpValidationPlugin is IValidation, IValidationHook revert NotImplemented(); } + function validateSignature(uint8, address, bytes32, bytes calldata) external pure override returns (bytes4) { + revert NotImplemented(); + } + // Empty stubs function pluginMetadata() external pure override returns (PluginMetadata memory) {} diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index 5c2ecb63..71afbdbf 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -157,20 +157,41 @@ 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.validateSignature( + uint8(ISingleOwnerPlugin.FunctionId.SIG_VALIDATION), + address(this), + 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.validateSignature( + uint8(ISingleOwnerPlugin.FunctionId.SIG_VALIDATION), + address(this), + 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.validateSignature( + uint8(ISingleOwnerPlugin.FunctionId.SIG_VALIDATION), address(this), digest, signature + ), + _1271_MAGIC_VALUE + ); } }