💥 Add Strict ERC-1271 Signature Checking for `SignatureChecker`

[pcaversaccio]

💥 Add Strict ERC-1271 Signature Checking for `SignatureChecker`

[YamenMerhi]

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

[pcaversaccio]

@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 contract.

Case 2: Support of ECDSA & EIP-1271

Devs can use the SignatureChecker contract.

Case 3: Only EIP-1271 Support

Devs can use the SignatureChecker 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

@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

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?

[pcaversaccio]

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.

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

awesome thx