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.

Sign up for GitHub

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

Jump to bottom

fix: KeepKey produces accurate EIP-712 eth_signTypedData_v4 signatures #649

Merged
merged 19 commits into from
Oct 27, 2023
Merged

fix: KeepKey produces accurate EIP-712 eth_signTypedData_v4 signatures #649

merged 19 commits into from
Oct 27, 2023

Conversation

0xMillz
Copy link
Contributor

@0xMillz 0xMillz commented Oct 18, 2023 •
edited
Loading

This PR fixes ethSignTypedData for KeepKey, which was returning inaccurate, unverifiable signatures.

Officially closes the hdwallet part of issue #578 and is fully compatible with EIP-712.

This bug was apparent in issues like shapeshift/web#5384
after the JSON RPC eth_signTypedData signatures sent to OpenSea were being rejected as invalid.

Implementation details:
I went with the EthereumSignTypedHash approach for ALL TypedData messages after consulting with @BitHighlander. It seems that the KeepKey firmware is missing the EthereumTypedDataStructRequest type, which would be required for a fully human readable, eip-712-compatible message to be displayed on the device screen.

Fortunately, the EthereumSignTypedHash approach allows the KeepKey to at least sign the v4 TypedData messages, which for end-users that means being able to do modern web3 interactions like buying and selling NFTs on OpenSea, for example.

-The bulk of the fix is in packages/hdwallet-keepkey/src/ethereum.ts

  • I also added a recoverTypedSignature check to the sandbox so you know right away if the signature that any wallet spits out is accurate.
    -I refactored the Ethereum sandbox tests too. That file is getting way too large and repetitive, but still is the best way to test with KeepKey.
  • I couldn't help but fix some other random some bugs along the way.

Thanks!

image

*KeepKey Bridge didn't seem to be working for me in the sandbox so I used the regular webusb option instead.

@0xMillz
Fix ethSignTypedData so that the signing address can be recovered fro…
f847bf5 
…m the signature
@0xMillz
Update Sandbox ethSignTypedData to verify that signature is accurate …
1045c4a 
…by recovering the signing address + refactor the Ethereum Sandbox section + tech debt + random bug fixes caught along the way + messing around with tsconfig
@vercel
Copy link

vercel bot commented Oct 18, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hdwallet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 27, 2023 8:20am

@0xMillz 0xMillz changed the title fix: KeepKey produces accurate eth_signTypedData_v4 signatures fix: KeepKey produces accurate EIP-712 eth_signTypedData_v4 signatures Oct 18, 2023
@BitHighlander
Copy link
Contributor

BitHighlander commented Oct 18, 2023 •
edited
Loading

This is tested and confirmed to be working. great work @millsmcilroy

v4 types data is current broken for both shapeshift web and kk desktop users. this is a required pr for addressing and fixing. Issues are demonstrable by using v4 typed data over wallet connect.

👍 :shipit:

@0xMillz
Copy link
Contributor Author

0xMillz commented Oct 18, 2023

This is tested and confirmed to be working. great work @millsmcilroy

v4 types data is current broken for both shapeshift web and kk desktop users. this is a required pr for addressing and fixing. Issues are demonstrable by using v4 typed data over wallet connect.

👍 :shipit:

ShapeShift team, @0xApotheosis, @gomesalexandre, et al., I'll just standby and wait for a PR review. When it's all ✅ and ready to ship I can update with a version bump or leave that to you. Thx!

This was referenced Oct 18, 2023
Closed
Closed
@0xMillz
Revert "Merge branch 'master' into fix_kk_ethSignTypedData"
b5fa182 
This reverts commit d1ab981, reversing
changes made to c4766cf.

Reverts accidental blind merge from master
0xMillz
0xMillz commented Oct 23, 2023
View reviewed changes
examples/sandbox/index.ts Outdated Show resolved Hide resolved
0xMillz
0xMillz commented Oct 23, 2023
View reviewed changes
examples/sandbox/index.ts Outdated Show resolved Hide resolved
0xMillz
0xMillz commented Oct 24, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
@0xMillz
Revert "Revert "Merge branch 'master' into fix_kk_ethSignTypedData""
6b2ce3a 
This reverts commit b5fa182 and resolves all conflicts

Lesson for future: use `git reset` to "revert" a merge commit or bad things can happen
0xMillz
0xMillz commented Oct 24, 2023
View reviewed changes
tsconfig.json Outdated Show resolved Hide resolved
@kaladinlight
Copy link
Contributor

needs to be rebased against master and another package bump. logic looks sane outside of the couple small comments above

@0xMillz
Copy link
Contributor Author

0xMillz commented Oct 27, 2023

Should be g2g. Apparently running npx update-browserslist-db@latest makes sneaky changes to your yarn.lock. I had to manually remove them. Vercel deployment is passing now, but seems like the playground deployments haven't been able to pair wallets since before my PR. Works on my machine though!

@kaladinlight kaladinlight merged commit 5dccb7d into shapeshift:master Oct 27, 2023
3 checks passed
@0xApotheosis 0xApotheosis mentioned this pull request Oct 29, 2023
Merged
3 tasks
@0xMillz 0xMillz deleted the fix_kk_ethSignTypedData branch December 8, 2023 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaladinlight kaladinlight kaladinlight approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@0xMillz @BitHighlander @kaladinlight