Fix personal sign fallback, only do NestedPersonalSign

Author: zhongeric

snapshots/MinimalDelegationIsValidSignatureTest.json

@@ -1,12 +1,10 @@
 {
   "isValidSignature_P256": "41828",
-  "isValidSignature_P256_personalSign": "33738",
-  "isValidSignature_P256_personalSign_notNested_usingHashTypedData": "306194",
-  "isValidSignature_P256_typedData_notNested_safeERC1271Caller": "32853",
+  "isValidSignature_P256_personalSign": "33678",
+  "isValidSignature_P256_typedData_notNested_safeERC1271Caller": "32820",
   "isValidSignature_P256_withHook": "47986",
   "isValidSignature_WebAuthnP256": "48515",
   "isValidSignature_rootKey": "17370",
-  "isValidSignature_rootKey_personalSign": "9301",
-  "isValidSignature_rootKey_personalSign_notNested_usingHashTypedData": "13708",
-  "isValidSignature_rootKey_typedData_notNested_safeERC1271Caller": "8416"
+  "isValidSignature_rootKey_personalSign": "9241",
+  "isValidSignature_rootKey_typedData_notNested_safeERC1271Caller": "8383"
 }

snapshots/MinimalDelegationTest.json

@@ -1,5 +1,5 @@
 {
-  "minimalDelegationEntry bytecode size": "20946",
+  "minimalDelegationEntry bytecode size": "20885",
   "register": "184758",
   "revoke": "54560",
   "validateUserOp_missingAccountFunds": "64567",

src/ERC1271.sol

@@ -24,7 +24,7 @@ abstract contract ERC1271 is IERC1271, BaseAuthorization {
     function setERC1271CallerIsSafe(address caller, bool isSafe) external onlyThis {
         _erc1271CallerIsSafe[caller] = isSafe;
     }
-    
+
     /// @inheritdoc IERC1271
     function isValidSignature(bytes32 hash, bytes calldata signature) public view virtual returns (bytes4);
 }

src/MinimalDelegation.sol

@@ -201,18 +201,16 @@ contract MinimalDelegation is
         // Must be branched because we do abi decoding in memory which will throw since the encoding schemes are different
         // ECDSA signatures are 65 bytes while P256 signatures are 64 bytes
         if (signature.length == 64 || signature.length == 65) {
-            if(_isSafeERC1271Caller(msg.sender)) {
+            if (_isSafeERC1271Caller(msg.sender)) {
                 // If the caller is safe we can simply verify the key's signature over `data`
                 // Data is already hashed with the app's domain separator so we don't rehash
                 isValid = key.verify(data, signature);
             } else {
                 // Otherwise, we try to verify the signature using NestedPersonalSign
-                // falling back to regular PersonalSign
-                isValid = 
-                    _isValidNestedPersonalSignature(key, data, signature) 
-                    || key.verify(hashTypedData(data), signature);
+                isValid = _isValidNestedPersonalSignature(key, data, signature);
             }
         } else {
+            // If there is additional data in the signature we assume we are using the TypedDataSign path
             isValid = _isValidTypedDataSig(key, data, signature);
         }
         // Early return if the signature is invalid

test/MinimalDelegation.isValidSignature.t.sol

@@ -341,7 +341,7 @@ contract MinimalDelegationIsValidSignatureTest is DelegationHandler, HookHandler
         assertEq(result, _1271_MAGIC_VALUE);
     }
 
-    function test_isValidSignature_personalSign_notNested_andNotHashTypedData_isInvalid() public {
+    function test_isValidSignature_personalSign_notNested_isInvalid() public {
         string memory message = "test";
         bytes32 messageHash = MessageHashUtils.toEthSignedMessageHash(bytes(message));
         // Incorrectly do personal_sign instead of over the typed PersonalSign digest
@@ -352,21 +352,6 @@ contract MinimalDelegationIsValidSignatureTest is DelegationHandler, HookHandler
         assertEq(signerAccount.isValidSignature(messageHash, wrappedSignature), _1271_INVALID_VALUE);
     }
 
-    function test_isValidSignature_personalSign_p256Key_notNested_andNotHashTypedData_isInvalid() public {
-        TestKey memory p256Key = TestKeyManager.initDefault(KeyType.P256);
-        vm.prank(address(signerAccount));
-        signerAccount.register(p256Key.toKey());
-
-        string memory message = "test";
-        bytes32 messageHash = MessageHashUtils.toEthSignedMessageHash(bytes(message));
-        // Incorrectly do personal_sign instead of over the typed PersonalSign digest
-        bytes memory signature = p256Key.sign(messageHash);
-        bytes memory wrappedSignature = abi.encode(p256Key.toKeyHash(), signature, EMPTY_HOOK_DATA);
-        // Should return the invalid value
-        vm.prank(address(mockERC1271VerifyingContract));
-        assertEq(signerAccount.isValidSignature(messageHash, wrappedSignature), _1271_INVALID_VALUE);
-    }
-
     /// forge-config: default.isolate = true
     /// forge-config: ci.isolate = true
     function test_isValidSignature_rootKey_notNested_safeERC1271Caller_isValid_gas() public {
@@ -407,38 +392,4 @@ contract MinimalDelegationIsValidSignatureTest is DelegationHandler, HookHandler
         assertEq(result, _1271_MAGIC_VALUE);
         vm.snapshotGasLastCall("isValidSignature_P256_typedData_notNested_safeERC1271Caller");
     }
-
-    /// forge-config: default.isolate = true
-    /// forge-config: ci.isolate = true
-    function test_isValidSignature_rootKey_personalSign_notNested_usingHashTypedData_isValid_gas() public {
-        string memory message = "test";
-        bytes32 messageHash = MessageHashUtils.toEthSignedMessageHash(bytes(message));
-        // We don't do ERC-7739 NestedPersonalSign, but rather apply EIP-712 hashing over the message
-        // which is also accepted since it protects against replay attacks by using the account's domain separator
-        bytes memory signature = signerTestKey.sign(signerAccount.hashTypedData(messageHash));
-        bytes memory wrappedSignature = abi.encode(KeyLib.ROOT_KEY_HASH, signature, EMPTY_HOOK_DATA);
-        vm.prank(address(mockERC1271VerifyingContract));
-        // Should be valid and return the magic value
-        assertEq(signerAccount.isValidSignature(messageHash, wrappedSignature), _1271_MAGIC_VALUE);
-        vm.snapshotGasLastCall("isValidSignature_rootKey_personalSign_notNested_usingHashTypedData");
-    }
-
-    /// forge-config: default.isolate = true
-    /// forge-config: ci.isolate = true
-    function test_isValidSignature_p256Key_personalSign_notNested_usingHashTypedData_isValid_gas() public {
-        TestKey memory p256Key = TestKeyManager.initDefault(KeyType.P256);
-        vm.prank(address(signerAccount));
-        signerAccount.register(p256Key.toKey());
-
-        string memory message = "test";
-        bytes32 messageHash = MessageHashUtils.toEthSignedMessageHash(bytes(message));
-        // We don't do ERC-7739 NestedPersonalSign, but rather apply EIP-712 hashing over the message
-        // which is also accepted since it protects against replay attacks by using the account's domain separator
-        bytes memory signature = p256Key.sign(signerAccount.hashTypedData(messageHash));
-        bytes memory wrappedSignature = abi.encode(p256Key.toKeyHash(), signature, EMPTY_HOOK_DATA);
-        vm.prank(address(mockERC1271VerifyingContract));
-        // Should be valid and return the magic value
-        assertEq(signerAccount.isValidSignature(messageHash, wrappedSignature), _1271_MAGIC_VALUE);
-        vm.snapshotGasLastCall("isValidSignature_P256_personalSign_notNested_usingHashTypedData");
-    }
 }