Fix Misclassification of ECDSA Signatures in verifyMultisig() (#1973)

* Fix multisignature routing to properly detect signature type

* Improve testing suite

* update dep in tests

* lint

* revert changes to tests

* Add reprocible test

---------

Co-authored-by: tarikgul <tariksnow37@gmail.com>

packages/util-crypto/src/signature/verify.spec.ts

@@ -7,6 +7,7 @@ import { hexToU8a, stringToU8a, u8aConcat, u8aToHex, u8aWrapBytes } from '@polka
 import { waitReady } from '@polkadot/wasm-crypto';
 
 import { decodeAddress } from '../address/index.js';
+import { secp256k1Sign } from '../secp256k1/sign.js';
 import { signatureVerify } from './index.js';
 
 const ADDR_ED = 'DxN4uvzwPzJLtn17yew6jEffPhXQfdKHTp2brufb98vGbPN';
@@ -37,7 +38,7 @@ describe('signatureVerify', (): void => {
   });
 
   describe('verifyDetect', (): void => {
-    it('verifies an ed25519 signature', (): void => {
+    it('verifies ed25519 signature', (): void => {
       expect(signatureVerify(MESSAGE, SIG_ED, ADDR_ED)).toEqual({
         crypto: 'ed25519',
         isValid: true,
@@ -46,7 +47,7 @@ describe('signatureVerify', (): void => {
       });
     });
 
-    it('verifies an ecdsa signature', (): void => {
+    it('verifies ecdsa signatures', (): void => {
       expect(signatureVerify(MESSAGE, SIG_EC, ADDR_EC)).toEqual({
         crypto: 'ecdsa',
         isValid: true,
@@ -173,11 +174,57 @@ describe('signatureVerify', (): void => {
 
     it('fails on an invalid signature', (): void => {
       expect(signatureVerify(MESSAGE, MUL_SR, ADDR_ED)).toEqual({
-        crypto: 'sr25519',
+        crypto: 'none',
         isValid: false,
         isWrapped: false,
         publicKey: new Uint8Array([61, 12, 55, 211, 0, 211, 97, 199, 4, 37, 17, 213, 81, 175, 166, 23, 251, 199, 144, 210, 19, 83, 186, 1, 196, 231, 14, 156, 171, 46, 141, 146])
       });
     });
+    /**
+     * ref: https://github.com/polkadot-js/common/issues/1898
+     *
+     * The following test ensures that we cover a reproduction that showed
+     * an inherent issue with verifying ecdsa signatures which is fixed in
+     * https://github.com/polkadot-js/common/pull/1973.
+     *
+     * It uses a random secretKey, and publicKey pair along with `secp256k1Sign`
+     * as the signer which is used for `ecdsa`.
+     */
+    it('Ensure ecdsa can sign and verify 1000 messages', (): void => {
+      const verifyThousandMessages = () => {
+        const secretKey = new Uint8Array([
+          103, 97, 114, 98, 97, 103, 101, 32, 114, 105, 100,
+          103, 101, 32, 107, 105, 99, 107, 32, 114, 111, 115,
+          101, 32, 101, 110, 100, 32, 115, 113, 117, 101
+        ]);
+        const publicKey = new Uint8Array([
+          2, 179, 102, 92, 246, 50, 172, 88,
+          81, 116, 8, 211, 192, 131, 154, 184,
+          122, 83, 180, 104, 4, 227, 214, 195,
+          140, 11, 82, 229, 49, 211, 185, 176,
+          63
+        ]);
+
+        for (let i = 0; i < 1000; i++) {
+          const message = `message ${i}`;
+          const encodedMessage = stringToU8a(message);
+          const signature = secp256k1Sign(encodedMessage, { secretKey });
+
+          const { isValid: valid } = signatureVerify(
+            message,
+            signature,
+            publicKey
+          );
+
+          if (!valid) {
+            return false;
+          }
+        }
+
+        return true;
+      };
+
+      expect(verifyThousandMessages()).toEqual(true);
+    });
   });
 });

packages/util-crypto/src/signature/verify.ts

@@ -31,13 +31,10 @@ const VERIFIERS_ECDSA: Verifier[] = [
 
 const VERIFIERS: Verifier[] = [
   ['ed25519', ed25519Verify],
-  ['sr25519', sr25519Verify],
-  ...VERIFIERS_ECDSA
+  ['sr25519', sr25519Verify]
 ];
 
-const CRYPTO_TYPES: ('ed25519' | 'sr25519' | 'ecdsa')[] = ['ed25519', 'sr25519', 'ecdsa'];
-
-function verifyDetect (result: VerifyResult, { message, publicKey, signature }: VerifyInput, verifiers = VERIFIERS): VerifyResult {
+function verifyDetect (result: VerifyResult, { message, publicKey, signature }: VerifyInput, verifiers = [...VERIFIERS, ...VERIFIERS_ECDSA]): VerifyResult {
   result.isValid = verifiers.some(([crypto, verify]): boolean => {
     try {
       if (verify(message, signature, publicKey)) {
@@ -56,25 +53,29 @@ function verifyDetect (result: VerifyResult, { message, publicKey, signature }:
 }
 
 function verifyMultisig (result: VerifyResult, { message, publicKey, signature }: VerifyInput): VerifyResult {
-  if (![0, 1, 2].includes(signature[0])) {
+  if (![0, 1, 2].includes(signature[0]) || ![65, 66].includes(signature.length)) {
     throw new Error(`Unknown crypto type, expected signature prefix [0..2], found ${signature[0]}`);
   }
 
-  const type = CRYPTO_TYPES[signature[0]] || 'none';
+  // If the signature is 66 bytes it must be an ecdsa signature
+  // containing: prefix [1 byte] + signature [65] bytes.
+  // Remove the and then verify
+  if (signature.length === 66) {
+    result = verifyDetect(result, { message, publicKey, signature: signature.subarray(1) }, VERIFIERS_ECDSA);
+  } else {
+    // The signature contains 65 bytes which is either
+    // - A ed25519 or sr25519 signature [1 byte prefix + 64 bytes]
+    // - An ecdsa signature [65 bytes]
+    result = verifyDetect(result, { message, publicKey, signature: signature.subarray(1) }, VERIFIERS);
 
-  result.crypto = type;
+    if (!result.isValid) {
+      result = verifyDetect(result, { message, publicKey, signature }, VERIFIERS_ECDSA);
+    }
 
-  try {
-    result.isValid = {
-      ecdsa: () => verifyDetect(result, { message, publicKey, signature: signature.subarray(1) }, VERIFIERS_ECDSA).isValid,
-      ed25519: () => ed25519Verify(message, signature.subarray(1), publicKey),
-      none: () => {
-        throw Error('no verify for `none` crypto type');
-      },
-      sr25519: () => sr25519Verify(message, signature.subarray(1), publicKey)
-    }[type]();
-  } catch {
-    // ignore, result.isValid still set to false
+    // If both failed, explicitly set crypto to 'none'
+    if (!result.isValid) {
+      result.crypto = 'none';
+    }
   }
 
   return result;