Repository: pcaversaccio/snekmate (language: python)

Issue:
💥 Add Strict ERC-1271 Signature Checking for `SignatureChecker`
## What this issue is about?

### Motivation

In the current `SignatureChecker`, there is support to check if a signature is valid with the `is_valid_signature_now(..)` function that checks the following:
- If the `hash` and `signature` recover an address that is the same as the `signer`, it returns true.
- When it's not the case, it calls `is_valid_signature(..)` on the `signer` assuming it's a contract. 

What would be a great addition to the library is to have another internal function that checks if a signature is valid with **ERC1271** only. So calling `is_valid_signature(..)` directly on the signer implementing ERC1271.

This function will be useful for functions that want to interact only with wallets such as argent, gnosis, etc .. and check the signature **only via ERC1271**. 

Then this function can be re-used in `is_valid_signature_now(..)`.

### Details

Developers can interact with other smart contract wallets by calling `is_valid_signature(..)` function directly. Still, they need to handle the return value, so the new function can be a wrapper named `is_valid_ERC1271_signature_now(address,bytes32,bytes[65])` that calls `is_valid_signature(..)` on the address with the hash and the signature and don't revert if the function doesn't exist and return true or false, depending if the function returns the `isValidSignature.selector`. Exactly like what the code does in the second part of the `is_valid_signature_now(..)` function.

If you find this addition relevant, I can work on creating the new function (splitting the `is_valid_signature_now(..)` function into a new one).

### Code example that solves the feature:

It would be mostly taking this part of the function to a new function (`is_valid_ERC1271_signature_now(..)`) that can be re-used in `is_valid_signature_now(..)`
```vy
# Second check: EIP-1271 case.
    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)))
```



Discussion / hints:
@YamenMerhi thanks for sharing the details about your idea. Your idea addresses the composability of the code. While I do see the intent of your issue, I don't think this is the right approach to solve this. Let me explain why:

#### Case 1: Only ECDSA Support

Devs can use the [`ECDSA`](https://github.com/pcaversaccio/snekmate/blob/main/src/utils/ECDSA.vy) contract.

#### Case 2: Support of ECDSA & EIP-1271

Devs can use the [`SignatureChecker`](https://github.com/pcaversaccio/snekmate/blob/main/src/utils/ECDSA.vy) contract.

#### Case 3: Only EIP-1271 Support

Devs can use the [`SignatureChecker`](https://github.com/pcaversaccio/snekmate/blob/main/src/utils/ECDSA.vy) contract with a simple `is_contract` check beforehand and making the function `_is_valid_signature_now` `internal` or simply using the `external` function via a simple interface:


*Option 1:* `internal` function approach 
```vy
@external
@view
def is_valid_ERC1271_signature_now(signer: address, hash: bytes32, signature: Bytes[65]) -> bool:
    if (signer.is_contract):
        return self._is_valid_signature_now(signer, hash, signature)
    return False


@internal
@view
def _is_valid_signature_now(signer: address, hash: bytes32, signature: Bytes[65]) -> bool:
    # First check: ECDSA case.
    recovered: address = self._recover_sig(hash, signature)
    if (recovered == signer):
        return True

    # Second check: EIP-1271 case.
    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)))
```

*Option 2:* `external` function approach

```vyper
interface SignatureChecker:
    def is_valid_signature_now(signer: address, hash: bytes32, signature: Bytes[65]) -> bool: view


@external
@view
def is_valid_ERC1271_signature_now(signer: address, hash: bytes32, signature: Bytes[65]) -> bool:
    if (signer.is_contract):
        return SignatureChecker(self).is_valid_signature_now(signer, hash, signature)
    return False
```

The contract `SignatureChecker` is a superset of `ECDSA` & `EIP-1271` and is already composable if you need to strictly make a `EIP-1271` check. Would agree with this or why exactly do you see a specific need to solve this via a separate `internal` function?
 
I think after reflecting on your proposal, it would make sense to add two functions:
- `is_valid_ERC1271_signature_now` (`external`) that simply calls the following `internal` function
- `_is_valid_ERC1271_signature_now` (`internal`)

`_is_valid_ERC1271_signature_now` can be re-used in `is_valid_signature_now`.

```vyper
@external
@view
def is_valid_ERC1271_signature_now(signer: address, hash: bytes32, signature: Bytes[65]) -> bool:
    return self._is_valid_ERC1271_signature_now(signer, hash, signature)


@internal
@view
def _is_valid_ERC1271_signature_now(signer: address, hash: bytes32, signature: Bytes[65]) -> bool:
    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)))


@external
@view
def is_valid_signature_now(signer: address, hash: bytes32, signature: Bytes[65]) -> bool:
    # First check: ECDSA case.
    recovered: address = self._recover_sig(hash, signature)
    if (recovered == signer):
        return True

    # Second check: EIP-1271 case.
    return self._is_valid_ERC1271_signature_now(signer, hash, signature)
```

Feel free to open a PR on that including the necessary test cases.

@pcaversaccio Exactly, will create the PR for it soon 👍 
awesome thx

Fix patch:
diff --git a/README.md b/README.md
index 9d92344c..fc0477b6 100644
--- a/README.md
+++ b/README.md
@@ -27,7 +27,7 @@ src
    ├─ CreateAddress — "`CREATE` EVM Opcode Utility Function for Address Calculation"
    ├─ Create2Address — "`CREATE2` EVM Opcode Utility Functions for Address Calculations"
    ├─ ECDSA — "Elliptic Curve Digital Signature Algorithm (ECDSA) Functions"
-   ├─ SignatureChecker — "ECDSA and EIP-1271 Signature Verification Function"
+   ├─ SignatureChecker — "ECDSA and EIP-1271 Signature Verification Functions"
    ├─ EIP712DomainSeparator — "EIP-712 Domain Separator"
    ├─ MerkleProofVerification — "Merkle Tree Proof Verification Functions"
    ├─ Multicall — "Multicall Functions"
diff --git a/src/utils/SignatureChecker.vy b/src/utils/SignatureChecker.vy
index 5e8bf39c..8631122b 100644
--- a/src/utils/SignatureChecker.vy
+++ b/src/utils/SignatureChecker.vy
@@ -1,14 +1,17 @@
 # @version ^0.3.7
 """
-@title ECDSA and EIP-1271 Signature Verification Function
+@title ECDSA and EIP-1271 Signature Verification Functions
 @license GNU Affero General Public License v3.0
 @author pcaversaccio
-@notice Signature verification helper that can be used instead
-        of `ECDSA.recover_sig` to seamlessly support both ECDSA
-        signatures from externally-owned accounts (EOAs) as well
-        as EIP-1271 (https://eips.ethereum.org/EIPS/eip-1271)
-        signatures from smart contract wallets like Argent and Gnosis Safe.
-        The implementation is inspired by OpenZeppelin's implementation here:
+@notice Signature verification helper functions that can be used
+        instead of `ECDSA.recover_sig` to seamlessly support both
+        ECDSA signatures from externally-owned accounts (EOAs) as
+        well as EIP-1271 (https://eips.ethereum.org/EIPS/eip-1271)
+        signatures from smart contract wallets like Argent and Gnosis
+        Safe. For strict EIP-1271 verification, i.e. no ECDSA signatures
+        of EOAs are accepted, the function `is_valid_ERC1271_signature_now`
+        can be called. The implementation is inspired by OpenZeppelin's
+        implementation here:
         https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/SignatureChecker.sol.
 @custom:security Signatures must not be used as unique identifiers since the
                  `ecrecover` opcode allows for malleable (non-unique) signatures.
@@ -55,6 +58,44 @@ def is_valid_signature_now(signer: address, hash: bytes32, signature: Bytes[65])
         return True
 
     # Second check: EIP-1271 case.
+    return self._is_valid_ERC1271_signature_now(signer, hash, signature)
+
+
+@external
+@view
+def is_valid_ERC1271_signature_now(signer: address, hash: bytes32, signature: Bytes[65]) -> bool:
+    """
+    @dev Checks if a signature `signature` is valid
+         for a given `signer` and message digest `hash`.
+         The signature is validated using EIP-1271.
+    @notice Unlike ECDSA signatures, contract signatures
+            are revocable and the result of this function
+            can therefore change over time. It could return
+            `True` in block N and `False` in block N+1 (or the opposite).
+    @param hash The 32-byte message digest that was signed.
+    @param signature The secp256k1 64/65-byte signature of `hash`.
+    @return bool The verification whether `signature` is valid
+            for the provided data.
+    """
+    return self._is_valid_ERC1271_signature_now(signer, hash, signature)
+
+
+@internal
+@view
+def _is_valid_ERC1271_signature_now(signer: address, hash: bytes32, signature: Bytes[65]) -> bool:
+    """
+    @dev This `internal` function is equivalent to
+         `is_valid_ERC1271_signature_now`, and can be used
+         for strict EIP-1271 verification.
+    @notice Unlike ECDSA signatures, contract signatures
+            are revocable and the result of this function
+            can therefore change over time. It could return
+            `True` in block N and `False` in block N+1 (or the opposite).
+    @param hash The 32-byte message digest that was signed.
+    @param signature The secp256k1 64/65-byte signature of `hash`.
+    @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)))


Test patch:
diff --git a/test/utils/SignatureChecker.t.sol b/test/utils/SignatureChecker.t.sol
index b881d7bc..fa06213a 100644
--- a/test/utils/SignatureChecker.t.sol
+++ b/test/utils/SignatureChecker.t.sol
@@ -32,6 +32,13 @@ contract SignatureCheckerTest is Test {
         assertTrue(
             signatureChecker.is_valid_signature_now(alice, hash, signature)
         );
+        assertTrue(
+            !signatureChecker.is_valid_ERC1271_signature_now(
+                alice,
+                hash,
+                signature
+            )
+        );
     }
 
     function testEOAWithInvalidSigner() public {
@@ -42,6 +49,13 @@ contract SignatureCheckerTest is Test {
         assertTrue(
             !signatureChecker.is_valid_signature_now(alice, hash, signature)
         );
+        assertTrue(
+            !signatureChecker.is_valid_ERC1271_signature_now(
+                alice,
+                hash,
+                signature
+            )
+        );
     }
 
     function testEOAWithInvalidSignature1() public {
@@ -57,6 +71,13 @@ contract SignatureCheckerTest is Test {
                 signatureInvalid
             )
         );
+        assertTrue(
+            !signatureChecker.is_valid_ERC1271_signature_now(
+                alice,
+                hash,
+                signatureInvalid
+            )
+        );
     }
 
     function testEOAWithInvalidSignature2() public {
@@ -66,6 +87,35 @@ contract SignatureCheckerTest is Test {
         bytes memory signatureInvalid = abi.encodePacked(r, s, bytes1(0xa0));
         vm.expectRevert(bytes("ECDSA: invalid signature"));
         signatureChecker.is_valid_signature_now(alice, hash, signatureInvalid);
+        assertTrue(
+            !signatureChecker.is_valid_ERC1271_signature_now(
+                alice,
+                hash,
+                signatureInvalid
+            )
+        );
+    }
+
+    function testEOAWithTooHighSValue() public {
+        address alice = vm.addr(1);
+        bytes32 hash = keccak256("WAGMI");
+        (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, hash);
+        uint256 sTooHigh = uint256(s) +
+            0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0;
+        bytes memory signatureInvalid = abi.encodePacked(
+            r,
+            bytes32(sTooHigh),
+            v
+        );
+        vm.expectRevert(bytes("ECDSA: invalid signature 's' value"));
+        signatureChecker.is_valid_signature_now(alice, hash, signatureInvalid);
+        assertTrue(
+            !signatureChecker.is_valid_ERC1271_signature_now(
+                alice,
+                hash,
+                signatureInvalid
+            )
+        );
     }
 
     function testEIP1271WithValidSignature() public {
@@ -79,6 +129,13 @@ contract SignatureCheckerTest is Test {
                 signature
             )
         );
+        assertTrue(
+            signatureChecker.is_valid_ERC1271_signature_now(
+                address(wallet),
+                hash,
+                signature
+            )
+        );
     }
 
     function testEIP1271WithInvalidSigner() public {
@@ -92,6 +149,13 @@ contract SignatureCheckerTest is Test {
                 signature
             )
         );
+        assertTrue(
+            !signatureChecker.is_valid_ERC1271_signature_now(
+                address(wallet),
+                hash,
+                signature
+            )
+        );
     }
 
     function testEIP1271WithInvalidSignature1() public {
@@ -106,6 +170,13 @@ contract SignatureCheckerTest is Test {
                 signatureInvalid
             )
         );
+        assertTrue(
+            !signatureChecker.is_valid_ERC1271_signature_now(
+                address(wallet),
+                hash,
+                signatureInvalid
+            )
+        );
     }
 
     function testEIP1271WithInvalidSignature2() public {
@@ -118,6 +189,12 @@ contract SignatureCheckerTest is Test {
             hash,
             signatureInvalid
         );
+        vm.expectRevert(bytes("ECDSA: invalid signature"));
+        signatureChecker.is_valid_ERC1271_signature_now(
+            address(wallet),
+            hash,
+            signatureInvalid
+        );
     }
 
     function testEIP1271WithMaliciousWallet() public {
@@ -131,5 +208,12 @@ contract SignatureCheckerTest is Test {
                 signature
             )
         );
+        assertTrue(
+            !signatureChecker.is_valid_ERC1271_signature_now(
+                address(malicious),
+                hash,
+                signature
+            )
+        );
     }
 }
diff --git a/test/utils/interfaces/ISignatureChecker.sol b/test/utils/interfaces/ISignatureChecker.sol
index 01e88a60..d2479a2c 100644
--- a/test/utils/interfaces/ISignatureChecker.sol
+++ b/test/utils/interfaces/ISignatureChecker.sol
@@ -7,4 +7,10 @@ interface ISignatureChecker {
         bytes32 hash,
         bytes memory signature
     ) external view returns (bool);
+
+    function is_valid_ERC1271_signature_now(
+        address signer,
+        bytes32 hash,
+        bytes memory signature
+    ) external view returns (bool);
 }