Fix Sign Transaction Signature #125

MiguelLZPF · 2024-05-06T10:40:04Z

Description

Changed the transaction bytes to be signed. More details available in the issue.

Fixes #124

Signed-off-by: Miguel_LZPF <[email protected]>

franfernandez20 · 2024-05-22T07:51:26Z

I recommend resolving this issue by reverting the implementation to Transaction.toBytes() and Transaction.fromBytes(). This approach not only resolves the problem but also eliminates the need to use private SDK methods. Additionally, it restores the capability to set multiple nodeIds.
Consequently, we should update the definition in the WalletConnect documentation for hedera_signTransaction to reflect these changes.

bugbytesinc · 2024-05-22T13:33:45Z

The multiple node id concept pulls against the intent of the wallet connect infrastructure, it is the source of other bugs and potential future exploits. Also, supporting multiple transactions in this could break conformity with HIP-820. The private methods are used because part of the constraints when creating the library was that "the JS sdk could not be changed in any way".

bugbytesinc · 2024-05-22T13:34:26Z

The real fix is to fix the JS SDK first, so it can support the proper wallet connect functionality.

gregscullard · 2024-05-23T08:23:24Z

Would it make sense to retain the existing method and add support for .toBytes serialised transactions as an additional method for backwards compatibility ? A decision to deprecate (or not) the current implementation can be made in the future.

The multiple node id concept pulls against the intent of the wallet connect infrastructure

How is this the case ? Wallet connect establishes a secure tunnel between a dApp and a wallet, it doesn't (to my knowledge) prescribe what content can or can't be transported over the protocol. Wallet connect became chain agnostic for this very reason (it used to prescribe eth transactions only).

it is the source of other bugs and potential future exploits.

to/fromBytes is used extensively, if there are bugs they should be fixed. If a dApp is concerned over the use of multiple nodes, it can choose to specify a single one or delegate the node selection to the wallet per https://hips.hedera.com/hip/hip-745 or continue using the current method (subject to deprecation decision per comment above)

Also, supporting multiple transactions in this could break conformity with HIP-820.

Standards aren't meant to be static, HIP-820 can be augmented to include this alternative method.

The private methods are used because part of the constraints when creating the library was that "the JS sdk could not be changed in any way".

The SDK couldn't be changed in time for the first release of wallet connect, it can of course be changed to meet the needs of the developer community.

gregscullard · 2024-05-23T09:52:55Z

I recommend resolving this issue by reverting the implementation to Transaction.toBytes() and Transaction.fromBytes().

Given this PR suggests a change to how a transactionBody is "extracted" from a Transaction, suggest we keep the conversation here relevant to this change and discuss support for to/fromBytes separately.

The prior implementation extracted a transactionBody from the Transaction and created a new body using a nodeAccountId that was selected at random by the library, thereby changing the transaction body prior to requesting a signature.

For example: If a developer created a transaction using the SDK, specified exactly one node he/she wanted to use, the library arbitrarily selected a different node (it could be the same thanks to randomness, but no guarantee)
The proposed change remains faithful to the developer's node selection.

gregscullard · 2024-05-23T09:54:18Z

src/lib/shared/utils.ts

-  transaction: T,
-  nodeAccountId: AccountId,
-) {
+export function transactionToTransactionBody<T extends Transaction>(transaction: T) {


I'm uncomfortable changing the signature of a public method, this could result in a breaking change to existing implementations.

Maybe this implementation should remain and Signer.ts changed from

const transactionBody: proto.TransactionBody = transactionToTransactionBody( transaction, this._getRandomNodes(1)[0], )

to re-using the nodeAccountId from transaction (if that's possible) ?

This would no longer be a breaking change.

Thanks for the feedback! 🙂

From our testing and research, the method used in this PR is the only way to get the signed transaction validated by the Hedera network. The previous approach signed the transaction in a way that the Hedera Network couldn't verify.

That said, we can leave the nodeAccountId parameter for compatibility, although it's currently unused and likely not functional in its previous form. @gregscullard, should I include the nodeAccountId as you suggested?

If there is a better method to obtain the signed transaction bytes than the one used in this PR, please let me know. Based on our testing a few weeks ago, this was the only approach that worked.

I'll leave it to the community of builders/users to determine whether a breaking change in the signature of the transaction is acceptable or not. The change is minor and would likely be implemented as a result of changes to the application, so tested and working before deployed. Breaking but not critically so I guess.

test/dapp/DAppConnector.test.ts

Signed-off-by: Miguel_LZPF <[email protected]>

tmctl

added another comment about accessing _signedTransactions, would the transaction body be in that array before a transaction is signed?

package.json

tmctl · 2024-08-14T12:33:43Z

src/lib/shared/utils.ts

 ) {
  // This is a private function, though provides the capabilities to construct a proto.TransactionBody
  //@ts-ignore
-  return transaction._makeTransactionBody(nodeAccountId)
+  return transaction._signedTransactions.current.bodyBytes


This utility function is used to get the transaction body before the transaction is signed, based on the name of _signedTransactions, I'd imagine this internal function only has the transactions in this array if the transaction has already been signed?

If so, we may need to try another method

@MiguelLZPF

Hi @tmctl! if I understood you correctly, there is a problem getting the signed transaction body here because this utility function was intended to get the transaction body not the signed transaction body.
So, the original issue is that HederaWalletConnect is getting the transaction body also when it should be looking for the signed transaction body.
So the solution is to have two separate utility functions then, right?

Added in the latest commit, take a look at it and tell me what you guys think 🤔

I've reverted the change; the issue was with the naming. transaction._signedTransactions.current.bodyBytes actually contains the UNSIGNED transaction body bytes, which makes sense since this occurs before the transaction is signed. It was confusing initially because of the misleading name, as you pointed out in your previous message.

This is the signed transaction bytes: signedTransaction._transactions.current.signedTransactionBytes 🫣. In my opinion is mega-difficult to understand this structure...

…n-signature Signed-off-by: Miguel_LZPF <[email protected]>

Signed-off-by: Miguel_LZPF <[email protected]>

…n-signature

MiguelLZPF · 2024-09-24T09:56:20Z

Just to add more clarity about this topic, this are our testing results for only sign Hedera SDK Transaction Objects:

  const fullTxBytes = transaction.toBytes();
  const transactionBytes = transaction._signedTransactions.get(0)!.bodyBytes!; //* <--
  const signedTransactionBytes = signedTransaction._transactions.get(0)!.signedTransactionBytes!;
  const signatures = signedTransaction.getSignatures().get(newAccountId)!;
  const transactionHash = await signedTransaction.getTransactionHash();
console.log(`
    Full Transaction: ${uint8ArrayToHex(fullTxBytes)}
    Transaction: ${uint8ArrayToHex(transactionBytes)}
    Signed Transaction: ${uint8ArrayToHex(signedTransactionBytes)}
    Tx Hash: ${uint8ArrayToHex(transactionHash)}
    Signature: ${uint8ArrayToHex(signatures.get(privateKey.publicKey)!)
')

MiguelLZPF added 3 commits May 6, 2024 12:39

fix: change body bytes to sign

24c3020

Signed-off-by: Miguel_LZPF <[email protected]>

fix: test sign transaction

7725c6e

Signed-off-by: Miguel_LZPF <[email protected]>

fix: removed unused AccountId

35b0d58

Signed-off-by: Miguel_LZPF <[email protected]>

MiguelLZPF marked this pull request as ready for review May 6, 2024 10:59

Merge branch 'main' into fix/sign-transaction-signature

a47c21d

hgraphql requested review from franfernandez20, teacoat, valeraOlexienko, a team and bugbytesinc and removed request for a team May 21, 2024 19:50

gregscullard reviewed May 23, 2024

View reviewed changes

gregscullard requested changes May 24, 2024

View reviewed changes

test/dapp/DAppConnector.test.ts Outdated Show resolved Hide resolved

fix: remove irrelevant comment

a20ba76

Signed-off-by: Miguel_LZPF <[email protected]>

MiguelLZPF requested a review from a team as a code owner May 28, 2024 07:00

MiguelLZPF and others added 2 commits May 28, 2024 09:22

Merge branch 'main' into fix/sign-transaction-signature

9a3a23d

fix: main branch change test

6382906

Signed-off-by: Miguel_LZPF <[email protected]>

MiguelLZPF mentioned this pull request Jul 29, 2024

Error verifying the signature using DAppConnector.signTransaction() #124

Open

Merge branch 'main' into fix/sign-transaction-signature

f46d34f

Signed-off-by: Miguel_LZPF <[email protected]>

MiguelLZPF requested a review from a team as a code owner July 29, 2024 11:37

feat: get latest changes from main & minor changes

b9e1e0d

Signed-off-by: Miguel_LZPF <[email protected]>

MiguelLZPF requested review from a team as code owners July 29, 2024 13:21

MiguelLZPF requested review from dalvizu and jeromy-cannon July 29, 2024 13:21

tmctl reviewed Aug 14, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix/sign-transactio…

5259eb5

…n-signature Signed-off-by: Miguel_LZPF <[email protected]>

MiguelLZPF requested a review from a team as a code owner September 19, 2024 11:26

MiguelLZPF added 3 commits September 19, 2024 13:36

feat: add separate function to get the SIGNED transaction body

85cd2b4

Signed-off-by: Miguel_LZPF <[email protected]>

fix: revert last commit 85cd2b4

fbc74d5

Signed-off-by: Miguel_LZPF <[email protected]>

Merge remote-tracking branch 'upstream/main' into fix/sign-transactio…

9a5dd89

…n-signature

jeromy-cannon approved these changes Sep 30, 2024

View reviewed changes

jeromy-cannon self-requested a review September 30, 2024 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Sign Transaction Signature #125

Fix Sign Transaction Signature #125

MiguelLZPF commented May 6, 2024 •

edited

Loading

franfernandez20 commented May 22, 2024

bugbytesinc commented May 22, 2024

bugbytesinc commented May 22, 2024

gregscullard commented May 23, 2024

gregscullard commented May 23, 2024

gregscullard May 23, 2024

gregscullard May 23, 2024

MiguelLZPF May 23, 2024

gregscullard May 24, 2024

tmctl left a comment

tmctl Aug 14, 2024

MiguelLZPF Sep 19, 2024

MiguelLZPF Sep 19, 2024

MiguelLZPF Sep 24, 2024

MiguelLZPF Sep 24, 2024

MiguelLZPF commented Sep 24, 2024

Fix Sign Transaction Signature #125

Are you sure you want to change the base?

Fix Sign Transaction Signature #125

Conversation

MiguelLZPF commented May 6, 2024 • edited Loading

Description

franfernandez20 commented May 22, 2024

bugbytesinc commented May 22, 2024

bugbytesinc commented May 22, 2024

gregscullard commented May 23, 2024

gregscullard commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmctl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelLZPF commented Sep 24, 2024

MiguelLZPF commented May 6, 2024 •

edited

Loading