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

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

Open
MiguelLZPF opened this issue May 6, 2024 · 8 comments · Fixed by #125
Open

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

MiguelLZPF opened this issue May 6, 2024 · 8 comments · Fixed by #125
Labels
Bug A error that causes the feature to behave differently than what was expected based on design docs P1 High priority issue. Required to be completed in the assigned milestone.

Comments

@MiguelLZPF
Copy link
Contributor

Describe the bug A clear and concise description of what the bug is.

In our testing of the signTransaction functionality from ioBuilders for integration into the Hedera Stablecoin project, we have encountered an issue with signature verification. The current implementation fails to verify signatures within our testing environment. Interestingly, we have successfully verified signatures using Blade, HasPack, and Metamask wallets.

In an attempt to address this, we modified our code to verify transactions created with hedera-wallet-connect against a customized verification function. However, when these transactions were submitted to the Hedera Network for verification, they were rejected. This discrepancy leads us to suspect that the signTransaction method may be producing incorrect signatures, possibly due to inaccuracies in the signed bytes generated.

To Reproduce Steps to reproduce the behavior:

  1. Clone the Hedera Stablecoin repository
  2. Setup Stablecoin with a backend for multi-signature instance
  3. Use a multi-signature account to create a cash in transaction (for example)
  4. Use a Wallet Connect connection wallet to sign the previously created transaction
  5. Verification exception should be thrown from the backend

Expected behavior A clear and concise description of what you expected to happen.

The same verification process should verify signatures from hedera-wallet-connect.

Desktop (please complete the following information):

  • OS: MacOS
  • Browser: Brave (chrome)
@tmctl
Copy link
Contributor

tmctl commented Jul 24, 2024

@MiguelLZPF did this PR solve the issue for you - #170

the most recent sdk version has these changes.

If not, we'll continue to test the PR you've added

@MiguelLZPF
Copy link
Contributor Author

Hey Tyler @tmctl!
Yes, we checked it last week but didn't have the chance to try it out yet. I believe it improves how the node account is managed, with the main change being the payload that needs to be signed, if I recall correctly.

I'll try the new version today. If we still need the changes in this PR, I'll update it with the latest modifications. Thank you for the heads up.

By "most recent SDK version," you're referring to the Hedera Wallet Connect repository, right?

@MiguelLZPF
Copy link
Contributor Author

Unfortunately, PR #170 does not provide a correct signature, resulting in an "Invalid signature" error with version 1.3.2-1 of hedera-wallet-connect. Therefore, we will incorporate the changes into PR #125.

@tmctl
Copy link
Contributor

tmctl commented Jul 30, 2024

okay, thanks @MiguelLZPF

I'll prioritize getting this over the line

@MiguelLZPF
Copy link
Contributor Author

To provide more clarity, the main issue is that the payload being signed is incorrect. As a result, when attempting to validate the signature, the two hashes do not match, leading to an invalid signature. The node ID is irrelevant in this context, maybe if there were more than one signature or whatever it is relevant, I don't know.

Additionally, based on my experience with the code, the tests currently in place only check if something is signed. They do not verify the validity of the signature. Therefore, these test signatures are likely invalid. I think that it is a good idea to implement tests that actually verify the signatures to ensure their correctness.

@itsbrandondev itsbrandondev added Bug A error that causes the feature to behave differently than what was expected based on design docs P1 High priority issue. Required to be completed in the assigned milestone. labels Aug 3, 2024
@hgraphql hgraphql reopened this Nov 7, 2024
@hgraphql
Copy link
Contributor

hgraphql commented Nov 7, 2024

There is an open PR to revert the change that fixed this issue, though has been reported to not work with wallets in production.

#325

This issue remains to find a solution that both works with the current implementation without breaking changes and works with StableCoin Studio - https://github.com/hashgraph/stablecoin-studio

Let's add a PR into that repository if that is what is needed.

@MiguelLZPF
Copy link
Contributor Author

Hi! Great. It might be worth taking a closer look at this issue, as simply rolling back the changes may not be the final solution. Before PR 126, the Hedera Testnet itself couldn’t validate a signed transaction created with this method. Ideally, we can identify the underlying issue and work toward a real solution where the signatures are fully valid. @hgraphql

@kantorcodes
Copy link
Contributor

@MiguelLZPF

By any chance could you provide me with a set of minimally reproducible code snippets so we can try this irregardless of implementation, e.g. the most bare bones example outside of stablecoin studio? I am looking into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A error that causes the feature to behave differently than what was expected based on design docs P1 High priority issue. Required to be completed in the assigned milestone.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants