Add strict ERC1271 Signature Checking in SignatureChecker library

[YamenMerhi]

🧐 Motivation

In the current SignatureChecker library, there is support to check if a signature is valid with the isValidSignatureNow(..) 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 isValidSignature(..) 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 isValidSignature(..) directly on the signer.

This function will be useful for functions that want to check the signature only via ERC1271. Then this internal function can be re-used in isValidSignatureNow(..).

📝 Details

The only thing that I would argue is that the newly created internal function, e.g isValidERC1271SignatureNow(..) should return the standard 4 bytes that will be returned by the function in case it's there, and if the function doesn't exist we can return the 0xffffffff fail value.

function isValidERC1271SignatureNow(address contract, bytes32 dataHash, bytes memory signature) public view returns (bytes4) {

      (bool success, bytes memory result) = contract.staticcall(
            abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature)
        );
  
         if (success) return result.length != 0 ? abi.decode(result, (bytes4)) : bytes4(0xffffffff);
         else return 0xffffffff;
}

Code might not be working, just drafting the concept

In this way, people can use directly in isValidSignature standard function:

function isValidSignature(bytes32 dataHash, bytes memory signature) public view returns (bytes4) {
        address _extensionContract = extension1271();  
        return SignatureChecker.isValidERC1271SignatureNow(_extensionContract, dataHash, signature);
}

instead of having the following:

function isValidERC1271SignatureNow(address contract, bytes32 dataHash, bytes memory signature) public view returns (bool) {
  
        (bool success, bytes memory result) = signer.staticcall(
            abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature)
        );
        return (success &&
            result.length == 32 &&
            abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector));
}

In this way, people are doing the work twice in the standard function:

function isValidSignature(bytes32 dataHash, bytes memory signature) public view returns (bytes4) {
        address _extensionContract = extension1271();  
        bool validSig = SignatureChecker.isValidERC1271SignatureNow(_extensionContract, dataHash, signature);
        if(validSig) return 0x1626ba7e; // IERC1271.isValidSignature.selector
        else return 0xffffffff; 
}

The code above is doing the work twice, first in isValidERC1271SignatureNow, we are comparing the returnValue to return a boolean, and then in isValidSignature(..) we are checking the boolean again to return the bytes4 value.

This function will be mainly used for signature checking related to ERC1271, if you think this issue is relevant, let's discuss.

[frangio]

I'm confused by the code snippets, what is extension1271?

If someone is only interested in 1271 signatures, why not directly call isValidSignature?

[YamenMerhi]

Sorry @frangio , my code snippets confused you, the whole idea is to have an internal function that:

• Calls isValidSignature(..) on an address with a staticcall.
• Return the isValidSignature(..) return value in case the function exists.
• Return 0xffffffff in case the function doesn't exist or the fallback function is/isn't existing.

In this way, people can use it without having to do the low-level call again and handle the return value so it doesn't revert if the function doesn't exist.

[frangio]

so it doesn't revert if the function doesn't exist.

Got it, this makes sense.

I'm less sure about the proposed return values. Why shouldn't we just return a boolean?

[Amxx]

I also think this should return a boolean.

function isValidERC1271SignatureNow(address signer, bytes32 dataHash, bytes memory signature) public view returns (bool) {
  
        (bool success, bytes memory result) = signer.staticcall(
            abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature)
        );
        return (success &&
            result.length == 32 &&
            abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector));
}

looks good to me.

[YamenMerhi]

@frangio @Amxx You're right, I was wrong about the return value, it's better to keep it as a boolean.
If you agree about this function add, I can create the PR 👍

[Zatacka]

is still dont understand what extension1271 is?

[YamenMerhi]

@Zatacka It was just an example to showcase how isValidERC1271SignatureNow(..) can be used:

• In some cases, you may need to call isValidSignature(..) on a contract. Making IERC1271(signer).isValidSignature(..) is not ideal as you want to track the return value, and see if the call reverts or not, and if the contract has a fallback function it will not revert but will not return the magic or fail value.

So instead of doing all of these checks, they could be provided by isValidERC1271SignatureNow(..) that will return true if the function return the magicValue, and false if all the other scenarios happens.