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

handle hex in personal signing for HW #1035

Merged
merged 2 commits into from
Oct 4, 2023
Merged

Conversation

brunobar79
Copy link
Member

@brunobar79 brunobar79 commented Oct 3, 2023

Fixes BX-1043
Figma link (if any):

What changed (plus any additional context for devs)

Don't blow up if the message is already a hex string

Screen recordings / screenshots

What to test

Personal Sign with trezor & ledger devices

@linear
Copy link

linear bot commented Oct 3, 2023

BX-1043 Updating personal_sign methods in Ledger and Trezor

In the non-hardware wallet flow, we have:

https://github.com/rainbow-me/browser-extension/blob/master/src/core/utils/signMessages.tsx#L35

Since some dapps send the message payload to sign as a utf8 string that is in hex form, we do a try/catch to convert hex>utf8. If this fails, then we leave the message as is.

However, in the Ledger and Trezor flows for personal_sign:

https://github.com/rainbow-me/browser-extension/blob/master/src/entries/popup/handlers/ledger.ts#L146

https://github.com/rainbow-me/browser-extension/blob/master/src/entries/popup/handlers/trezor.ts#L119

we don't do a try/catch around the toUtf8Bytes conversion so I expect this to fail.

The conversion attempt should be handled with a try/catch and fallback to using the original message as is during the failure case, so that the hex string will at least be displayed to the user if it is not a utf8 payload.

How to test:

Unfortunately there's no easy way to test this from the test dapps as the personal_sign on these dapps send over hex-encoded utf8 strings, but you can override the WC test dapp's personal_sign event and use this payload as an example to test locally:

["0x1c78982b939c88ca91014f810558a804f27f495a2a8c4198f494093c4af860d1", "0x2e67869829c734ac13723A138a952F7A8B56e774"]

Equivalent fixes on the app for reference: rainbow-me/rainbow#5071

Original user issue that brought this to our attention: rainbow-me/rainbow#5036

@brunobar79 brunobar79 requested a review from a team October 3, 2023 21:36
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Here's the packed extension for this build:
node_modules.tar.gz
rainbowbx-0fd8d47e54783f714fe5124fc58bf0b7b0e53b9f.zip

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Here's the packed extension for this build:
rainbowbx-1a1be60a5aab5480d9c84212bced361609057521.zip

@brunobar79 brunobar79 merged commit d70d135 into master Oct 4, 2023
15 checks passed
@brunobar79 brunobar79 deleted the @bruno/hw-signing-fixes branch October 4, 2023 16:03
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Here's the packed extension for this build:
rainbowbx-d70d135a7b8ffe6a396bc00cbcfc14d4bce3cb0e.zip

@BrodyHughes BrodyHughes mentioned this pull request Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants