Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong payload being signed #1442

Open
gagdiez opened this issue Dec 19, 2024 · 13 comments
Open

Wrong payload being signed #1442

gagdiez opened this issue Dec 19, 2024 · 13 comments

Comments

@gagdiez
Copy link
Contributor

gagdiez commented Dec 19, 2024

The current implementation of SignTransaction signs the transaction.encode(), which is the borsh serialized version of the transaction:

const encodedTx = transaction.encode();
const signedTransaction = new SignedTransaction({
transaction,
signature: new Signature({
keyType: transaction.publicKey.keyType,
data: await signer.signMessage(encodedTx),
}),
});

In reality, we need to sign the sha256 hash of the borsh serialized transaction:

const encodedTx = new Uint8Array(sha256.sha256.array(transaction.encode()));
@AlexKushnir1
Copy link

This means that signing is not working correctly. I don't fully understand how it worked beforehand and signed transactions were passed successfully and didn`t throw any errors?

@gagdiez
Copy link
Contributor Author

gagdiez commented Dec 21, 2024

@AlexKushnir1 @near-js/client is a relatively new package, which still has no proper testing

My guess is that nobody used it and detected it, or simply nobody reported the error

@AlexKushnir1
Copy link

AlexKushnir1 commented Jan 5, 2025

@gagdiez I opine it has sense to clarify that we expect hash of encodedTx in the signMessage. I see it like rewrite encodedTx into encodedTransactionHash and describe that signMessage() https://github.com/near/near-api-js/blob/c49fd67535baa5163b29ca7a43f585bfcfbe16e6/packages/client/src/interfaces/dependencies.ts#L10C1-L14C1 expects a hash of encoded transaction. WDYT?

@gagdiez
Copy link
Contributor Author

gagdiez commented Jan 5, 2025

I see that actually signMessage already applies the sha256, so maybe we need to clarify that on the signMessage doc String, and add a test to try all this actually works

@AlexKushnir1
Copy link

I just thought, if we specify on the signMessage that it should get an encoded hash it will cause a misleading naming.

I tested this func and related and there came to a conclusion that is"nt working, after adding hashing it was signed correctly and passed on the chain. Regarding the tests I would describe an issue and work for all client package

@AlexKushnir1
Copy link

I"m not sure I understand where is singMessage applies the sha256. I would be really grateful for clarifying it to me!

@gagdiez
Copy link
Contributor Author

gagdiez commented Jan 5, 2025

The inMemorySigner - I think this is the only one being implemented - hashes the message before signing it:

const hash = new Uint8Array(sha256(message)); 

async signMessage(message: Uint8Array, accountId?: string, networkId?: string): Promise<Signature> {
const hash = new Uint8Array(sha256(message));
if (!accountId) {
throw new Error('InMemorySigner requires provided account id');
}
const keyPair = await this.keyStore.getKey(networkId, accountId);
if (keyPair === null) {
throw new Error(`Key for ${accountId} not found in ${networkId}`);
}
return keyPair.sign(hash);
}
toString(): string {
return `InMemorySigner(${this.keyStore})`;
}
}

@AlexKushnir1
Copy link

AlexKushnir1 commented Jan 5, 2025

Sure, I didn`t test with the signer from keystore. THANKS!

Does the KeyPair signer still have an issue?

@gagdiez
Copy link
Contributor Author

gagdiez commented Jan 5, 2025

the KeyPair.sign does not apply the sha256 hash, and that is ok since the Key should not decide on how something should be signed

@AlexKushnir1
Copy link

Just tested like this:

async function testSigners() {
  const rpcProvider = getTestnetRpcProvider();
  const signer = getSignerFromPrivateKey(PRIVATE_KEY); 

  const {result, outcome} = await SignedTransactionComposer.init({
    sender: ACCOUNT_ID,
    receiver: 'alexkushnir.testnet',
    deps: { rpcProvider, signer },
  })
  .functionCall(
    "sign",
    Buffer.from(JSON.stringify({ request : {key_version:0,path:"test",payload:[12,1,2,0,4,5,6,8,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,44]}})),
    BigInt(300000000000000),
    BigInt(1)
  )
  .signAndSend();

  console.log(`${JSON.stringify(result, null)}`);
  console.log(`${JSON.stringify(outcome, null)}`);
}

And got the next errors

ServerError: Transaction is not signed with the given public key
    at parseRpcError (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/utils/lib/commonjs/errors/rpc_errors.cjs:25:19)
    at /home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/providers/lib/commonjs/json-rpc-provider.cjs:321:57
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async exponentialBackoff (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/providers/lib/commonjs/exponential-backoff.cjs:8:24)
    at async JsonRpcProvider.sendJsonRpc (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/providers/lib/commonjs/json-rpc-provider.cjs:306:26)
    at async FailoverRpcProvider.withBackoff (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/providers/lib/commonjs/failover-rpc-provider.cjs:54:32)
    at async SignedTransactionComposer.signAndSend (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/client/lib/commonjs/transactions/composers/signed_transaction_composer.cjs:96:25)
    at async testSubAccountCreation (/home/alex/Near/JS/test-api-js/src/index.js:47:33) {
  type: 'InvalidSignature',
  context: undefined
}
Switched to provider at the index 1
ServerError: Transaction is not signed with the given public key
    at parseRpcError (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/utils/lib/commonjs/errors/rpc_errors.cjs:25:19)
    at /home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/providers/lib/commonjs/json-rpc-provider.cjs:321:57
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async exponentialBackoff (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/providers/lib/commonjs/exponential-backoff.cjs:8:24)
    at async JsonRpcProvider.sendJsonRpc (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/providers/lib/commonjs/json-rpc-provider.cjs:306:26)
    at async FailoverRpcProvider.withBackoff (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/providers/lib/commonjs/failover-rpc-provider.cjs:54:32)
    at async SignedTransactionComposer.signAndSend (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/client/lib/commonjs/transactions/composers/signed_transaction_composer.cjs:96:25)
    at async testSubAccountCreation (/home/alex/Near/JS/test-api-js/src/index.js:47:33) {
  type: 'InvalidSignature',
  context: undefined
}
Switched to provider at the index 0
/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/providers/lib/commonjs/failover-rpc-provider.cjs:63
        throw new types_1.TypedError(`Exceeded ${this.providers.length} providers to execute request`, 'RetriesExceeded');
              ^

TypedError: Exceeded 2 providers to execute request
    at FailoverRpcProvider.withBackoff (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/providers/lib/commonjs/failover-rpc-provider.cjs:63:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async SignedTransactionComposer.signAndSend (/home/alex/Near/JS/test-api-js/node_modules/.pnpm/@[email protected]/node_modules/@near-js/client/lib/commonjs/transactions/composers/signed_transaction_composer.cjs:96:25)
    at async testSubAccountCreation (/home/alex/Near/JS/test-api-js/src/index.js:47:33) {
  type: 'RetriesExceeded',
  context: undefined
}

But when I pass hash function and compile js file:

async function signTransaction({ transaction, deps: { signer } }) {
    const encodedTx = new Uint8Array((0, sha256_1.sha256)(transaction.encode()));
    const signedTransaction = new transactions_1.SignedTransaction({
        transaction,
        signature: new transactions_1.Signature({
            keyType: transaction.publicKey.keyType,
            data: await signer.signMessage(encodedTx),
        }),
    });
    return {
        encodedTransactionHash: encodedTx,
        signedTransaction,
    };
}

Transaction is passing corectly

@gagdiez
Copy link
Contributor Author

gagdiez commented Jan 5, 2025

I see, then we do have a problem, because one signer implements it and another does not...

Either we make all signers hash the message, or we make all signers not hash the message, but having some doing it and some not is a huge problem

I think it is more transparent to make the signers not hash the message

Invoking @frol to see if he has an opinion here, since he built near-api-rs we could follow their take on this

@frol
Copy link
Collaborator

frol commented Jan 10, 2025

I agree with @gagdiez, the API has to be aligned - either expect the full message and hash it inside, or require the hashed message and then don't hash it inside the signer.

I believe that the signer method should expect the original message and hash and sign it; additionally, it may expose signHashedMessage (next to signMessage), so if anyone needs to have granular control over the hashing process, they can use this method.

@frol
Copy link
Collaborator

frol commented Jan 10, 2025

Ah, I think I know why that was implemented that way and creates this confusion:

signTransaction gets the transaction object and signMessage does not have a sense of "transaction", so at the very least, we have to serialize the object to borsh bytes, and then we can pass it there; but then why not hash it, right?... We need to think about this interface problem holistically and align the interfaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: NEW❗
Development

No branches or pull requests

3 participants