Repository: pcaversaccio/snekmate (language: python)

Issue:
🔒 Revert in EIP-1271 `SignatureChecker`
### Describe the issue:

### What this issue is about ?
Looking at the current implementation of `is_valid_ERC1271_signature_now(..)`, we can see that the return value is always checked to be the magic value in order to return true, otherwise, the function should return false. 

This is the same behavior implemented in OpenZeppelin's SignatureChecker library.

The only difference is that in the current implementation in vyper, if you call `is_valid_ERC1271_signature_now(..)` on a contract that doesn't implement `isValidSignature(..)` it will revert the call.

IMO, this should be changed to match the OpenZeppelin behavior, where in case of revert (The signer doesn't implement `isValidSignature(..)` nor have the default function) it should return false. 

Because the whole purpose of having a utility function like this is to abstract the checks that need to be done after an `isValidSignature(..)` call. The person implementing this function just uses it to know whether the signature was valid or invalid and shouldn't expect a revert.

Let me know what do you think.

### Code example to reproduce the issue:

This code snippet:
```vy
return_data: Bytes[32] = \
        raw_call(signer, _abi_encode(hash, signature, method_id=IERC1271_ISVALIDSIGNATURE_SELECTOR), max_outsize=32, is_static_call=True)
    return ((len(return_data) == 32) and (convert(return_data, bytes32) == convert(IERC1271_ISVALIDSIGNATURE_SELECTOR, bytes32)))
```

Needs to be changed to something like this: add the `revert_on_failure` flag, and in the return line add that it should be a `success and ..`
```vy
success: bool = False
    response: Bytes[32] = b""
    success, response = \
    raw_call(signer, _abi_encode(hash, signature, method_id=IERC1271_ISVALIDSIGNATURE_SELECTOR), max_outsize=32, is_static_call=True, revert_on_failure=False)
    return (success and (len(response) == 32) and (convert(response, bytes32) == convert(IERC1271_ISVALIDSIGNATURE_SELECTOR, bytes32)))
```


### Version:

`0.3.7+commit.6020b8b`

### Relevant log output:

_No response_


Discussion / hints:

Fix patch:
diff --git a/src/utils/SignatureChecker.vy b/src/utils/SignatureChecker.vy
index d4e842ee..2254a52b 100644
--- a/src/utils/SignatureChecker.vy
+++ b/src/utils/SignatureChecker.vy
@@ -96,9 +96,18 @@ def _is_valid_ERC1271_signature_now(signer: address, hash: bytes32, signature: B
     @return bool The verification whether `signature` is valid
             for the provided data.
     """
-    return_data: Bytes[32] = \
-        raw_call(signer, _abi_encode(hash, signature, method_id=IERC1271_ISVALIDSIGNATURE_SELECTOR), max_outsize=32, is_static_call=True)
-    return ((len(return_data) == 32) and (convert(return_data, bytes32) == convert(IERC1271_ISVALIDSIGNATURE_SELECTOR, bytes32)))
+    success: bool = empty(bool)
+    return_data: Bytes[32] = b""
+    # The following low-level call does not revert, but instead
+    # returns `False` if the callable contract does not implement
+    # the `isValidSignature` function. Since we perform a length
+    # check of 32 bytes for the return data in the return expression
+    # at the end, we also return `False` for EOA wallets instead
+    # of reverting (remember that the EVM always considers a call
+    # to an EOA as successful with return data `0x`).
+    success, return_data = \
+        raw_call(signer, _abi_encode(hash, signature, method_id=IERC1271_ISVALIDSIGNATURE_SELECTOR), max_outsize=32, is_static_call=True, revert_on_failure=False)
+    return (success and (len(return_data) == 32) and (convert(return_data, bytes32) == convert(IERC1271_ISVALIDSIGNATURE_SELECTOR, bytes32)))
 
 
 @internal


Test patch:
diff --git a/test/utils/SignatureChecker.t.sol b/test/utils/SignatureChecker.t.sol
index fa06213a..01124f10 100644
--- a/test/utils/SignatureChecker.t.sol
+++ b/test/utils/SignatureChecker.t.sol
@@ -189,11 +189,12 @@ contract SignatureCheckerTest is Test {
             hash,
             signatureInvalid
         );
-        vm.expectRevert(bytes("ECDSA: invalid signature"));
-        signatureChecker.is_valid_ERC1271_signature_now(
-            address(wallet),
-            hash,
-            signatureInvalid
+        assertTrue(
+            !signatureChecker.is_valid_ERC1271_signature_now(
+                address(wallet),
+                hash,
+                signatureInvalid
+            )
         );
     }
 
@@ -216,4 +217,24 @@ contract SignatureCheckerTest is Test {
             )
         );
     }
+
+    function testEIP1271NoIsValidSignatureFunction() public {
+        bytes32 hash = keccak256("WAGMI");
+        (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, hash);
+        bytes memory signature = abi.encodePacked(r, s, v);
+        assertTrue(
+            !signatureChecker.is_valid_signature_now(
+                address(vyperDeployer),
+                hash,
+                signature
+            )
+        );
+        assertTrue(
+            !signatureChecker.is_valid_ERC1271_signature_now(
+                address(vyperDeployer),
+                hash,
+                signature
+            )
+        );
+    }
 }