Intermittent "invalid point" error when signing a transaction

[nedgar]

I'm getting frequent yet intermittent "invalid point" errors when using v1.1.2 to sign a transaction.

The signing algorithm for the asymmetric signing key in GCP KMS is Elliptic Curve P-256 / SHA256 -- using either version 1 or 2 here (version 3 was just to test, not being used):

The code derived from the example in the README:

require("dotenv").config();
const { KeyManagementServiceClient } = require("@google-cloud/kms");
const { ethers } = require("ethers");
const { GcpKmsSigner } = require("ethers-gcp-kms-signer");

const bignum = ethers.BigNumber.from;

const { GOOGLE_APPLICATION_CREDENTIALS, PROJECT_ID, LOCATION_ID, KEYRING_ID, INFURA_URL } =
  process.env;
...
async function main(id = "eth-signing-key-1") {
  // const client = new KeyManagementServiceClient();

  const kmsCredentials = {
    projectId: PROJECT_ID,
    locationId: LOCATION_ID,
    keyRingId: KEYRING_ID,
    keyId: id,
    keyVersion: "1",
  };

  const tx = {
    to:
      "0xE94E130546485b928C9C9b9A5e69EB787172952e", // from example in README at https://github.com/johanneskares/ethers-gcp-kms-signer
    value: ethers.utils.parseEther("0.01").toString(),
    gasPrice: 3000000000,
    gasLimit: 500000,
  };
  console.log("tx to sign:", tx);

  const popTx = await signer.populateTransaction(tx);
  console.log("populated tx:", popTx);
  console.log("balance required (ETH):", ethers.utils.formatEther(bignum(popTx.gasLimit).mul(popTx.maxFeePerGas).add(popTx.value)));

  console.log("signing tx...");
  let signedTx;
  for (let i = 0; i < 10; ++i) {
    try {
      signedTx = await signer.signTransaction(popTx);
      break
    } catch (err) {
      console.error(`Error in signTransaction on attempt ${i+1}`, err);
      if (err == 9) {
        throw err;
      }
    }
  }
  console.log("signed tx:", signedTx);

  console.log("sending signed tx...");
  const sentTx = await provider.sendTransaction(signedTx);
  console.log("sentTx:", sentTx);

  const receipt = await sentTx.wait();
  console.log("receipt:", receipt);
}
...

When it fails, the stack is:

Error: invalid point
at ShortCurve.pointFromX (/Users/nedgar/src/my-first-project/node_modules/elliptic/lib/elliptic/curve/short.js:195:11)
at EC.recoverPubKey (/Users/nedgar/src/my-first-project/node_modules/elliptic/lib/elliptic/ec/index.js:215:20)
at recoverPublicKey (/Users/nedgar/src/my-first-project/node_modules/@ethersproject/signing-key/lib/index.js:61:30)
at Object.recoverAddress (/Users/nedgar/src/my-first-project/node_modules/@ethersproject/transactions/lib/index.js:72:62)
at recoverPubKeyFromSig (/Users/nedgar/src/my-first-project/node_modules/ethers-gcp-kms-signer/dist/util/gcp-kms-utils.js:163:31)
at determineCorrectV (/Users/nedgar/src/my-first-project/node_modules/ethers-gcp-kms-signer/dist/util/gcp-kms-utils.js:176:16)
at /Users/nedgar/src/my-first-project/node_modules/ethers-gcp-kms-signer/dist/signer.js:68:69
at Generator.next (<anonymous>)
at asyncGeneratorStep (/Users/nedgar/src/my-first-project/node_modules/ethers-gcp-kms-signer/dist/signer.js:22:103)
at _next (/Users/nedgar/src/my-first-project/node_modules/ethers-gcp-kms-signer/dist/signer.js:24:194)

Yesterday this was happening 80-90% of the time. Today I haven't been able to reproduce it. Very odd.

[nedgar]

The package dependencies are as follows. I restricted ethers and kms to the versions ethers-gcp-kms-signer 1.1.2 specifies, but that didn't make any difference.

"@google-cloud/kms": "2.10.0",
"dotenv": "^16.0.1",
"ethers": "5.5.1",
"ethers-gcp-kms-signer": "1.1.2"

[nedgar]

Yesterday this was happening 80-90% of the time. Today I haven't been able to reproduce it. Very odd.

I had switched to 1.1.1, but now it's failing with that too, in the same way, about 50% of the time.

[nedgar]

I'm running Node v14.20.0. FYI yarn complains about crypto package not being needed: warning crypto@1.0.1: This package is no longer supported. It's now a built-in Node module. If you've depended on crypto, you should switch to the one that's built-in.

[nedgar]

I was able to reproduce the problem in the current repository, by adding this to src/signer.test.ts as the first test. It's basically the "should send a signed transaction" test but with sendTransaction changed to signTransaction.

  test("should sign a transaction", async () => {
    const provider = ethers.providers.InfuraProvider.getWebSocketProvider("rinkeby", process.env.INFURA_KEY);

    const signer = new GcpKmsSigner(kmsCredentials).connect(provider);

    const tx = await signer.signTransaction({
      to: "0xEd7B3f2902f2E1B17B027bD0c125B674d293bDA0",
      value: ethers.utils.parseEther("0.001"),
    });

    expect(tx).not.toBeNull();

    /* eslint-disable no-console */
    console.log(tx);
  });

It passed on first run but failed on second:

[fforbeck]

hey @nedgar . Thanks for flagging this. Have you tested with the Elliptic Curve secp256k1 key SHA256 Digest algo? (You need to enable the HSM with Asymmetric sign).

[nedgar]

@fforbeck Thanks, the secp256k1 algo works much better, lol. I don't know how I missed that option. Do you think the signer could be made to "fail fast" if the incorrect algorithm is chosen (without extra round trips for the happy path)? I notice the response includes the protection level, e.g. I added console.log("requestKmsSignature - response:", response) in requestKmsSignature:

Or perhaps we can just add an API function that checks whether the algorithm for a given key is compatible?

[fforbeck]

@nedgar yeah, that's the algo Ethereum uses. Good point, I believe if we implement your suggestion from #6 (instantiating the kms client only once) we reduce the leak, and we can execute an additional call to get the configs (kms.getCryptoKeyVersion) to verify if it is using the correct algorithm. If I remember correctly, the kms.getPublicKey also returns the algo, but we are not checking that. Feel free to submit a PR with these changes, happy to review that.

[nedgar]

That sounds good, I’ll work on a PR in the coming week. For now, I just moved the kms const assignment outside the functions. On Aug 10, 2022, 9:26 AM -0400, Felipe Forbeck @.***>, wrote:

@nedgar yeah, that's the algo Ethereum uses. Good point, I believe if we implement your suggestion from #6 (instantiating the kms client only once) we reduce the leak, and we can execute an additional call to get the configs (kms.getCryptoKeyVersion) to verify if it is using the correct algorithm. If I remember correctly, the kms.getPublicKey also returns the algo, but we are not checking that. Feel free to submit a PR with these changes, happy to review that. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>