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

.changeset/many-humans-begin.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`SignatureChecker`: address issue where the identity precompile would be recognized as a valid signer for hash that start with 28 bytes of zero.

contracts/utils/cryptography/SignatureChecker.sol

@@ -45,6 +45,7 @@ library SignatureChecker {
         );
         return (success &&
             result.length >= 32 &&
-            abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector));
+            abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector) &&
+            signer != address(0x4));
     }
 }

test/helpers/precompiles.js

@@ -0,0 +1,12 @@
+module.exports = {
+  ecRecover: '0x0000000000000000000000000000000000000001',
+  SHA2_256: '0x0000000000000000000000000000000000000002',
+  RIPEMD_160: '0x0000000000000000000000000000000000000003',
+  identity: '0x0000000000000000000000000000000000000004',
+  modexp: '0x0000000000000000000000000000000000000005',
+  ecAdd: '0x0000000000000000000000000000000000000006',
+  ecMul: '0x0000000000000000000000000000000000000007',
+  ecPairing: '0x0000000000000000000000000000000000000008',
+  blake2f: '0x0000000000000000000000000000000000000009',
+  pointEvaluation: '0x000000000000000000000000000000000000000a',
+};

test/utils/cryptography/SignatureChecker.test.js

@@ -2,6 +2,8 @@ const { ethers } = require('hardhat');
 const { expect } = require('chai');
 const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
 
+const precompile = require('../../helpers/precompiles');
+
 const TEST_MESSAGE = ethers.id('OpenZeppelin');
 const TEST_MESSAGE_HASH = ethers.hashMessage(TEST_MESSAGE);
 
@@ -25,35 +27,52 @@ describe('SignatureChecker (ERC1271)', function () {
 
   describe('EOA account', function () {
     it('with matching signer and signature', async function () {
-      expect(await this.mock.$isValidSignatureNow(this.signer, TEST_MESSAGE_HASH, this.signature)).to.be.true;
+      await expect(this.mock.$isValidSignatureNow(this.signer, TEST_MESSAGE_HASH, this.signature)).to.eventually.be
+        .true;
     });
 
     it('with invalid signer', async function () {
-      expect(await this.mock.$isValidSignatureNow(this.other, TEST_MESSAGE_HASH, this.signature)).to.be.false;
+      await expect(this.mock.$isValidSignatureNow(this.other, TEST_MESSAGE_HASH, this.signature)).to.eventually.be
+        .false;
     });
 
     it('with invalid signature', async function () {
-      expect(await this.mock.$isValidSignatureNow(this.signer, WRONG_MESSAGE_HASH, this.signature)).to.be.false;
+      await expect(this.mock.$isValidSignatureNow(this.signer, WRONG_MESSAGE_HASH, this.signature)).to.eventually.be
+        .false;
     });
   });
 
   describe('ERC1271 wallet', function () {
     for (const fn of ['isValidERC1271SignatureNow', 'isValidSignatureNow']) {
       describe(fn, function () {
         it('with matching signer and signature', async function () {
-          expect(await this.mock.getFunction(`$${fn}`)(this.wallet, TEST_MESSAGE_HASH, this.signature)).to.be.true;
+          await expect(this.mock.getFunction(`$${fn}`)(this.wallet, TEST_MESSAGE_HASH, this.signature)).to.eventually.be
+            .true;
         });
 
         it('with invalid signer', async function () {
-          expect(await this.mock.getFunction(`$${fn}`)(this.mock, TEST_MESSAGE_HASH, this.signature)).to.be.false;
+          await expect(this.mock.getFunction(`$${fn}`)(this.mock, TEST_MESSAGE_HASH, this.signature)).to.eventually.be
+            .false;
+        });
+
+        it('with identity precompile and random hash', async function () {
+          await expect(this.mock.getFunction(`$${fn}`)(precompile.identity, TEST_MESSAGE_HASH, this.signature)).to
+            .eventually.be.false;
+        });
+
+        it('with identity precompile and zero hash', async function () {
+          await expect(this.mock.getFunction(`$${fn}`)(precompile.identity, ethers.ZeroHash, this.signature)).to
+            .eventually.be.false;
         });
 
         it('with invalid signature', async function () {
-          expect(await this.mock.getFunction(`$${fn}`)(this.wallet, WRONG_MESSAGE_HASH, this.signature)).to.be.false;
+          await expect(this.mock.getFunction(`$${fn}`)(this.wallet, WRONG_MESSAGE_HASH, this.signature)).to.eventually
+            .be.false;
         });
 
         it('with malicious wallet', async function () {
-          expect(await this.mock.getFunction(`$${fn}`)(this.malicious, TEST_MESSAGE_HASH, this.signature)).to.be.false;
+          await expect(this.mock.getFunction(`$${fn}`)(this.malicious, TEST_MESSAGE_HASH, this.signature)).to.eventually
+            .be.false;
         });
       });
     }