feat: [v0.8-develop]: sig validation in interface (#59)

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

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];

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) {

src/interfaces/IPlugin.sol

@@ -94,6 +94,7 @@ struct PluginManifest {
     ManifestAssociatedFunction[] validationFunctions;
     ManifestAssociatedFunction[] preValidationHooks;
     ManifestExecutionHook[] executionHooks;
+    uint8[] signatureValidationFunctions;
 }
 
 interface IPlugin is IERC165 {

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);
 }

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);
 }

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.

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;
     }
 

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 {

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

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) {}
 

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
+        );
     }
 }