Test behavior of SignatureChecker against the identity precompile (0x4)

[Amxx]

The identity precompiles, at address 0x4, is potentially dangerous when targeted by ERC1271 checks. The returned bytes contains starts with the function selector, which is exaclty the 4 bytes that are expected.

The current implementation (as of 5.2) checks the that the first 32 bytes of the returned value contain the selector followed by 28 bytes of zeros. This is what a valid ERC-1271 wallet would return when the call is successfull.

In the case of the identity precompile, the first 32 bytes of the returned value contain the selector followed by the first 28 bytes of the hash being verified. That means that unless the first 28 bytes of the hash are all zero, the precompile issue will not affect this library.

I real-life applications, the hash is actually the return of a hashing function. As a consequence, having 28 bytes of zeros is basically impossible, and the implmentation is not vulnerable.

However we cannot rule the case where some application decides the use a hash that is not an ERC-191 or ERC-712 hash, and is not produced by a hashing function. For this extermelly unlikelly case, we add an additional check that signer is not address(0x4).

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

⚠️ No Changeset found

Latest commit: f92d6d1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[arr00]

Need to update the commit message

[pcaversaccio]

I have a question @Amxx; you write here:

However we cannot rule the case where some application decides the use a hash that is not an ERC-191 or ERC-712 hash, and is not produced by a hashing function. For this extermelly unlikelly case, we add an additional check that signer is not address(0x4).

But I can't see any added checks in the Solidity implementation of this PR?

[Amxx]

It's was decided against it for a few reasons:

• we expect almost all usage will be on the isValidSignature now function, which is not affected
• we expect the hash to always be produced by a hashing function
• even if both are not true, we feel like having a valid signature for address 0x4 should not cause security issues. Address 0x4 should not have any permissions or hold any assets.

Overall, 99.99% of the usage are already safe and should not bear any extra cost. The remaining 0.01% are a combination of many things, one of which is bad application design.

[pcaversaccio]

we expect almost all usage will be on the isValidSignature now function, which is not affected

Well, the identity precompile will become a predeploy with code probably soon in the future: https://eips.ethereum.org/EIPS/eip-7666. Thus the following line in isValidSignatureNow, will become false:

openzeppelin-contracts/contracts/utils/cryptography/SignatureChecker.sol

Line 23 in f281e98

if (signer.code.length == 0) {

and isValidERC1271SignatureNow is called.

we expect the hash to always be produced by a hashing function

Fair.

[Amxx]

Wow, thanks for sharing eip-7666. That is one less "safeguard", but I'd say we likelly are still safe.

I'd really like to see a vulnerable application to better understand in what case 0x4 being a valid signer of something is an issue.

[pcaversaccio]

I'd really like to see a vulnerable application to better understand in what case 0x4 being a valid signer of something is an issue.

My issue here links to a recent hack due to inheriting the ERC-6492 reference implementation. That was the original trigger around this discussion.

[Amxx]

My issue here links to a recent hack due to inheriting the ERC-6492 reference implementation.

Thank you again for that !

Looking very quickly at https://pbs.twimg.com/media/GiByHVbacAAJQEX?format=jpg&name=4096x4096, I'm not sure I understand how 0x4 is involved here. If I was to deploy the ERC-7666 bytecode at any address that is NOT in the precompile range, using CREATE or CREATE2, and use that address instead of 0x4, wouldn't the vulnerability work just the same ? If 7666 goes through, I feel address 0x4 would be exactly the same as any such address (that has the same code deployed) ... so filterting address 0x4 (or even the range of precompile addresses) would not change anything, whould it?

EDIT: I'm partially wrong, with any other address, contractCodeLength would either not be 0, or it would have to be created by the arbitrary call. Why only "partially" wrong ? Well the arbitrary call could be malicious AND deploy the code as expected. For example, it could be an ERC-721, 1155, or 1363 "transfer with hook" and the hook would be able to deploy the fake signer. It would limit the range of calls that can be executed, but there would still be a vulnerability.

[pcaversaccio]

Re the 0x4 involvement, you can see this here:

I mean, the vulnerability - as you correctly state - could also be exploited by any arbitrary contract returning 0x1626ba7e (generally speaking); he just chose to use the identity precompile here. So having the arbitrary call possibility in combination with ERC-6492 introduces this vulnerability. Since ERC-6492 uses ERC-1271 our natural reaction was to look into this as well. If you delegate verification to another contract (which is done by ERC-1271) this can't be arbitrary, otherwise you're rekt of course. The overall question is if there are protocols using ERC-1271 with the wrong assumptions, i.e. if you directly call a ERC-1271 wallet (i.e. without ECDSA verification) will a EOA address always return 0x only?

[Amxx]

IMO the actual good fix for that is to use a SenderCreator similar the one used in ERC-4337 EntryPoint

[pcaversaccio]

Do you think it's worth linking this conversation or as a general code comment in the SignatureChecker contract?

[Amxx]

Do you think it's worth linking this conversation or as a general code comment in the SignatureChecker contract?

Maybe