The identity precompile can be used to bypass ERC-1271 signature checks

address(4) is the identity precompile, it returns whatever data it was called with. Calling isValidSignature(bytes32,bytes) on address(4) will return the selector of isValidSignature(bytes32,bytes) plus whatever arguments were provided.

Unfortunately, the ERC-1271 magic value is the selector for isValidSignature:

  // bytes4(keccak256("isValidSignature(bytes32,bytes)")
  bytes4 constant internal MAGICVALUE = 0x1626ba7e;

Therefore, it is always possible to pass address(4) as a signer and whatever bogus data to pass a isValidERC1271SignatureNow in SignatureChecker. Can you please confirm if this is intentional or if a workaround should be added?

Example:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;

import "forge-std/Test.sol";

import {SignatureChecker} from "openzeppelin-contracts/contracts/utils/cryptography/SignatureChecker.sol";

contract ERC1271Check is Test {
    function test_erc1271Check() external view {
        bool success =
            SignatureChecker.isValidERC1271SignatureNow({signer: address(4), hash: bytes32(0), signature: new bytes(0)});
        assert(success);
    }
}

With:
forge Version: 0.3.1-nightly
Commit SHA: 082104867cc0d587196eec715a75736d61dbd9fa
openzeppelin-contracts @ 828dbc357cd7975c0a31d919f6d4dcf7ef2dc427

Note that isValidSignatureNow does not have this issue, because address(4).code.length is zero.

Thank you!

2 Likes

Sounds like you are right, I am not sure for this, need more time to dig it out.

This method was introduced in this issue: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3912

1 Like

An important caveat to this issue is that the first 28 bytes of the hash must be zero which very unlikely when used to verify a real hash--that said, there are times where the hash will be some custom input. We will add a check to isValidERC1271SignatureNow that ensures the signer is not address(0x4).

2 Likes

There is a slight modification here. After further deliberation, we decided that this issue is a little too obscure to justify the added bytecode for the explicit check--as noted, it only occurs when using isValidERC1271SignatureNow with a hash prefixed by 28 bytes of 0.

We added a check to ensure that other hashes (not 28 bytes of 0) result in an invalid signature. You can see the conversion here.

1 Like