Skip to content

Incorrect V for EIP712 signature#152

Merged
digiwand merged 2 commits intoMetaMask:mainfrom
lambertkevin:bugfix/fix-v-in-sign-typed-data
Aug 24, 2022
Merged

Incorrect V for EIP712 signature#152
digiwand merged 2 commits intoMetaMask:mainfrom
lambertkevin:bugfix/fix-v-in-sign-typed-data

Conversation

@lambertkevin
Copy link
Copy Markdown
Contributor

@lambertkevin lambertkevin commented Jul 26, 2022

As described by #134 and #151, the V parsed by Metamask for a signTypedData is incorrect. The signature is not expected to be composed with the parity/recovery but with the classic 27 or 28.

This PR fixes this and updates the CI tests.

Thanks 🙏

@lambertkevin lambertkevin requested a review from a team as a code owner July 26, 2022 15:36
digiwand
digiwand reviewed Aug 17, 2022
View reviewed changes
@lambertkevin lambertkevin requested a review from digiwand August 22, 2022 14:48
@FrederikBolding
Copy link
Copy Markdown
Member

FrederikBolding commented Aug 22, 2022

@digiwand @lambertkevin Could we test this implementation against https://github.com/MetaMask/eth-sig-util to prevent issues like this in the future?

It may be out of scope for this PR.

@lambertkevin
Copy link
Copy Markdown
Contributor Author

lambertkevin commented Aug 22, 2022 •
edited
Loading

@digiwand @lambertkevin Could we test this implementation against https://github.com/MetaMask/eth-sig-util to prevent issues like this in the future?

It may be out of scope for this PR.

Hi @FrederikBolding !

Well the problem is that to my knowledge, @metamask/eth-sig-utils recoverPersonalSignature & recoverTypedSignature and ethers.js recoverAddress are all ignoring the value of v accepting 0 and 1 as a valid v because of EIP-1559, which means it doesn't matter what lib you're going to test the implementation against, you won't catch a v issue like this one. 🤔

And that's actually the reason why the issue is noticed only now that people start using ECDSA verification onchain with EIP-712 messages, because contrary to those libs, ecrecover is returning 0x0 with a v = 0 | 1. And since most of EIP-191 messages were only tested in frontends, no one had really catched the issue before 🤷‍♂️

digiwand
digiwand approved these changes Aug 22, 2022
View reviewed changes
Copy link
Copy Markdown

@digiwand digiwand left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the code with macOS x Chrome x Nano X using the test-dapp. LGTM! Thanks @lambertkevin

will hold on merging for @FrederikBolding, or another dev, to review

@digiwand digiwand merged commit dec4772 into MetaMask:main Aug 24, 2022
@lambertkevin lambertkevin deleted the bugfix/fix-v-in-sign-typed-data branch August 24, 2022 15:51
@xgambitox xgambitox mentioned this pull request Sep 7, 2022
Merged
@mcmire mcmire mentioned this pull request Jan 18, 2023
Closed
@cryptoKevinL
Copy link
Copy Markdown

cryptoKevinL commented Jan 21, 2023

I am seeing this issue as well, where signatures are different by decimal 27 between Ledger Live and Metamask+Ledger

@plasmacorral plasmacorral mentioned this pull request Jun 16, 2023
Closed
@plasmacorral plasmacorral mentioned this pull request Jul 12, 2023
Closed
@danjm danjm mentioned this pull request Nov 30, 2023
Closed
@plasmacorral plasmacorral mentioned this pull request Jan 6, 2024
Closed
@plasmacorral plasmacorral mentioned this pull request Feb 22, 2024
Closed
This was referenced Jan 29, 2026
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jpuri jpuri jpuri approved these changes

+1 more reviewer

@digiwand digiwand digiwand approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lambertkevin @FrederikBolding @cryptoKevinL @jpuri @digiwand