MetaMask
Fix encoding of array in signTypedData_v4
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!
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.
@danfinlay Anything we can do to help move this forward? This fix would be incredibly helpful for more complex EIP-712 signing
@SilentCicero will merging this fix impact Fuel's usage of eth_signTypedData_v4?
@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
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.
Agreed.
On Fri, Jan 22, 2021 at 1:59 PM Dan Finlay [email protected] 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 https://github.com/MetaMask/eth-sig-util/pull/107#issuecomment-765620780, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACK2CXTEESYAYWOBDCVKLPLS3HDIHANCNFSM4TDMDK6A .
@danfinlay @SilentCicero Happy to make the changes, let me know once a decision has been made.
Thanks
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.
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_v5for 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
@irux No, we are still waiting on changes to be made to this PR as per the suggestion above.
@irux @mcmire apologies, slipped out of my mind, I will try to get some time this weekend to make it ready!
@cammellos It wasn't meant to sound like a complaint or anything, more of just a statement, so no problem!
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.