Fix encoding of array in signTypedData_v4 #107

cammellos · 2020-10-29T08:03:46Z

Fixes: #106

Hello!

I was implementing signTypedData_v4 for our project, and I have used the tests in this repo to make sure our implementation was sound.
I then noticed that geth provided their own https://github.com/ethereum/go-ethereum/blob/43c278cdf93d5469702fd1c2f570dbf3c1718ff0/signer/core/signed_data.go#L323 , so I plugged in the tests from this repo in geth and noticed that the behavior was not consistent.

Upon further investigation, I noticed that currently the behavior of signTypedData_v4 is not according to
https://eips.ethereum.org/EIPS/eip-712 when it comes to encoding arrays.

The eip states:

The array values are encoded as the keccak256 hash of the
concatenated encodeData of their contents

The behavior instead was to encode array values as the keccak256 of the
concatenated keccak256 of the values.

This worked well for primary types, but not for struct, as encodeData
per spec is:

The encoding of a struct instance is enc(value₁) ‖ enc(value₂) ‖ … ‖ enc(valueₙ) , i.e. the concatenation of the
encoded member values in the order that they appear in the type.
Each encoded member value is exactly 32-byte long.

Instead, we were using basically hashStruct instead of encodeData

For reference here are the tests that I plugged in, taken from this repo:
https://github.com/status-im/status-go/blob/a7d1ba18f8be01920060503cc2fb99107aaee328/services/typeddata/metamask_test.go#L60

And here is the implementation, slightly changed so it can be passed a private key as a parameter, for testing, but is essentially the same:

https://github.com/status-im/status-go/blob/a7d1ba18f8be01920060503cc2fb99107aaee328/services/typeddata/sign.go#L46

In terms of coding, I tried to keep the changes to a minimum.
Also not sure if there's any compatibility problem with the existing libraries etc.

Btw, this is not quite my field of expertise, so I might have understood some behavior/lacking some context.

Thanks for the project!

danfinlay · 2020-10-29T17:26:09Z

Thanks for the report, we're sequencing the order of actions for ourselves now. We'll need to start by checking some metrics to confirm nobody is relying on the current behavior in production, and if anyone is, we'll need to provide a migration path for them.

tbtstl · 2020-11-29T20:34:02Z

@danfinlay Anything we can do to help move this forward? This fix would be incredibly helpful for more complex EIP-712 signing

rekmarks · 2020-12-31T18:46:31Z

@SilentCicero will merging this fix impact Fuel's usage of eth_signTypedData_v4?

SilentCicero · 2020-12-31T19:06:16Z

@rekmarks not sure, I'll have to do a check on that.

If it does, perhaps we can just leave this as a request-able functionality in metamask for Fuel (i.e. old_v4) or something.

Can we do a check before we merge?

This is currently how we use EIP712. Again, I built everything off of the existing available Spec libraries which was this:
https://github.com/FuelLabs/fuel-js/blob/master/packages/protocol/src/eip712.js

danfinlay · 2021-01-22T18:58:59Z

We want to have this improvement, but have been stymied by concerns that applications may be relying on the current behavior.

One path forward would be to package this fix as signTypedData_v5, and encourage clients that already have this fix as _v4 to simply also expose it as _v5 for the sake of recommending it as a more wallet-consistent implementation.

SilentCicero · 2021-01-22T19:13:58Z

Agreed.

…

On Fri, Jan 22, 2021 at 1:59 PM Dan Finlay ***@***.***> wrote: We want to have this improvement, but have been stymied by concerns that applications may be relying on the current behavior. One path forward would be to package this fix as signTypedData_v5, and encourage clients that already have this fix as _v4 to simply *also* expose it as _v5 for the sake of recommending it as a more wallet-consistent implementation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACK2CXTEESYAYWOBDCVKLPLS3HDIHANCNFSM4TDMDK6A> .

cammellos · 2021-01-26T06:06:06Z

@danfinlay @SilentCicero Happy to make the changes, let me know once a decision has been made.

Thanks

segun · 2021-12-02T18:11:24Z

Can you fix the conflicts?

cammellos · 2021-12-02T18:53:23Z

@segun sure, I should have some time tonight

cammellos · 2021-12-02T21:39:01Z

@segun rebased!

e00dan · 2021-12-02T21:50:44Z

src/sign-typed-data.ts

@@ -173,6 +173,7 @@ function encodeField(
  version: SignTypedDataVersion.V3 | SignTypedDataVersion.V4,
 ): [type: string, value: any] {
  validateVersion(version, [SignTypedDataVersion.V3, SignTypedDataVersion.V4]);
+  console.log('ENCODING', types, name, type, value);


This console.log probably should be removed before merging. 👍

whoops, forgot about that one :)

Gudahtt · 2021-12-02T21:54:46Z

Happy to make the changes, let me know once a decision has been made.

Sorry for the delay in replying! We would like this change to be introduced as signTypedData_v5 for backwards compatibility reasons. There are a few other breaking changes we hope to make at the same time to make this more spec-compliant, which I have created a milestone for.

Currently the behavior of signTypedData_v4 is not according to https://eips.ethereum.org/EIPS/eip-712 when it comes to encoding arrays. The eip states: "The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents". The behavior instead was to encode array values as the keccak256 of the concatenated keccak256 of the values. This worked well for primary types, but not for struct, as encodeData per spec is: "The encoding of a struct instance is enc(value₁) ‖ enc(value₂) ‖ … ‖ enc(valueₙ) , i.e. the concatenation of the encoded member values in the order that they appear in the type. Each encoded member value is exactly 32-byte long.". Instead, we were using basically `hashStruct` instead of `encodeData`

cammellos · 2021-12-02T22:07:39Z

Happy to make the changes, let me know once a decision has been made.

Sorry for the delay in replying! We would like this change to be introduced as signTypedData_v5 for backwards compatibility reasons. There are a few other breaking changes we hope to make at the same time to make this more spec-compliant, which I have created a milestone for.

Sure, sounds good,
should we keep the same pattern of having a single function and make decisions based on the passed version (in this case it would be a new one, v5)?
If so I can update the code and add tests specifically targeting v5

Gudahtt · 2021-12-03T09:56:20Z

Sure, that would be great!

irux · 2023-03-19T21:27:15Z

any news on this ?

mcmire · 2023-03-20T17:16:16Z

@irux No, we are still waiting on changes to be made to this PR as per the suggestion above.

cammellos · 2023-03-20T17:28:28Z

@irux @mcmire apologies, slipped out of my mind, I will try to get some time this weekend to make it ready!

mcmire · 2023-03-20T22:32:32Z

@cammellos It wasn't meant to sound like a complaint or anything, more of just a statement, so no problem!

adonesky1 · 2023-10-20T19:42:39Z

Should we re-visit this PR?

mcmire · 2023-10-20T19:54:57Z

I agree with @Gudahtt that we would want to copy v4 of signTypedData to a v5 and include this modification. This seems important and it should be our responsibility to handle this, so I'll make a note to create a new PR next week so that we can keep the ball rolling.

cammellos requested a review from a team as a code owner October 29, 2020 08:03

This was referenced Oct 29, 2020

Add support for signTypedData_v4 status-im/status-go#2062

Closed

personal_sign method does not work in status status-im/status-mobile#11333

Closed

cammellos force-pushed the bug/fix-signed-typed-data branch from cdcc0c9 to 9778b62 Compare October 29, 2020 08:28

RonSherfey approved these changes Apr 11, 2021

View reviewed changes

xlab mentioned this pull request May 28, 2021

Adding support of EIP-712 into Ethereum app trezor/trezor-firmware#1568

Closed

cammellos dismissed RonSherfey’s stale review via f52ebf7 December 2, 2021 21:36

cammellos force-pushed the bug/fix-signed-typed-data branch from 5d8f560 to f52ebf7 Compare December 2, 2021 21:36

e00dan suggested changes Dec 2, 2021

View reviewed changes

cammellos dismissed RonSherfey’s stale review via c3f3882 December 2, 2021 22:05

cammellos force-pushed the bug/fix-signed-typed-data branch from f52ebf7 to c3f3882 Compare December 2, 2021 22:05

mcmire dismissed RonSherfey’s stale review via c3f3882 April 5, 2022 15:22

Gudahtt dismissed RonSherfey’s stale review via c3f3882 April 6, 2022 13:41

Gudahtt dismissed stale reviews from RonSherfey via c3f3882 April 21, 2022 20:46

adonesky1 dismissed stale reviews from RonSherfey via c3f3882 September 13, 2022 16:02

ryanml dismissed RonSherfey’s stale review via c3f3882 September 22, 2022 23:03

adonesky1 dismissed RonSherfey’s stale review via c3f3882 September 28, 2022 15:59

kumavis dismissed RonSherfey’s stale review via c3f3882 November 9, 2022 01:09

Gudahtt dismissed stale reviews from RonSherfey via c3f3882 November 9, 2022 18:21

adonesky1 dismissed stale reviews from RonSherfey via c3f3882 November 9, 2022 22:40

Gudahtt dismissed RonSherfey’s stale review via c3f3882 December 2, 2022 13:44

Gudahtt dismissed RonSherfey’s stale review via c3f3882 January 9, 2023 13:53

Gudahtt dismissed RonSherfey’s stale review via c3f3882 February 6, 2023 14:36

danfinlay dismissed RonSherfey’s stale review via c3f3882 March 17, 2023 22:38

legobeat dismissed stale reviews from RonSherfey via c3f3882 April 21, 2023 13:10

legobeat dismissed stale reviews from RonSherfey via c3f3882 April 28, 2023 22:33

mcmire added the team-wallet-framework label Nov 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix encoding of array in signTypedData_v4 #107

Fix encoding of array in signTypedData_v4 #107

cammellos commented Oct 29, 2020 •

edited

Loading

danfinlay commented Oct 29, 2020 •

edited

Loading

tbtstl commented Nov 29, 2020

rekmarks commented Dec 31, 2020 •

edited

Loading

SilentCicero commented Dec 31, 2020 •

edited

Loading

danfinlay commented Jan 22, 2021

SilentCicero commented Jan 22, 2021 via email

cammellos commented Jan 26, 2021

segun commented Dec 2, 2021

cammellos commented Dec 2, 2021

cammellos commented Dec 2, 2021

e00dan Dec 2, 2021

cammellos Dec 2, 2021

Gudahtt commented Dec 2, 2021

cammellos commented Dec 2, 2021

Gudahtt commented Dec 3, 2021

irux commented Mar 19, 2023

mcmire commented Mar 20, 2023

cammellos commented Mar 20, 2023

mcmire commented Mar 20, 2023

adonesky1 commented Oct 20, 2023

mcmire commented Oct 20, 2023

Fix encoding of array in signTypedData_v4 #107

Are you sure you want to change the base?

Fix encoding of array in signTypedData_v4 #107

Conversation

cammellos commented Oct 29, 2020 • edited Loading

danfinlay commented Oct 29, 2020 • edited Loading

tbtstl commented Nov 29, 2020

rekmarks commented Dec 31, 2020 • edited Loading

SilentCicero commented Dec 31, 2020 • edited Loading

danfinlay commented Jan 22, 2021

SilentCicero commented Jan 22, 2021 via email

cammellos commented Jan 26, 2021

segun commented Dec 2, 2021

cammellos commented Dec 2, 2021

cammellos commented Dec 2, 2021

e00dan Dec 2, 2021

Choose a reason for hiding this comment

cammellos Dec 2, 2021

Choose a reason for hiding this comment

Gudahtt commented Dec 2, 2021

cammellos commented Dec 2, 2021

Gudahtt commented Dec 3, 2021

irux commented Mar 19, 2023

mcmire commented Mar 20, 2023

cammellos commented Mar 20, 2023

mcmire commented Mar 20, 2023

adonesky1 commented Oct 20, 2023

mcmire commented Oct 20, 2023

cammellos commented Oct 29, 2020 •

edited

Loading

danfinlay commented Oct 29, 2020 •

edited

Loading

rekmarks commented Dec 31, 2020 •

edited

Loading

SilentCicero commented Dec 31, 2020 •

edited

Loading