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

Cannot press sign button when call sign typed data v4 #4441

Closed
b21quocbao opened this issue Jun 3, 2022 · 15 comments
Closed

Cannot press sign button when call sign typed data v4 #4441

b21quocbao opened this issue Jun 3, 2022 · 15 comments
Assignees
Labels
type-bug Something isn't working WalletConnect WalletConnect related issue or bug

Comments

@b21quocbao
Copy link

Describe the bug
When call _signTypedData function from ethers metamask popup to sign the message shown up, Cancel button is pressable but the Sign button is unpressable. When I delete the field with an array type and then execute again, I can press the Sign button and it returns the transaction hash. I think the problem is due to typed data v4, which supports array.
Screenshots
If applicable, add screenshots or links to help explain your problem
image

To Reproduce
Steps to reproduce the behavior

  1. Call _signTypedData from ethers package or signTypedData from walletconnect package
  2. Popup from Metamask

Expected behavior
A sign button is pressable and returns the transaction hash

Smartphone (please complete the following information):

  • Device: Pixel 4
  • OS: Android 13
  • App Version 5.1.1
  • Framework: React Native

to be added after bug submission by internal support / PM
Severity

  • How critical is the impact of this bug on a user?
  • Add stats if available on % of customers impacted
  • Is this visible to all users?
  • Is this tech debt?
@b21quocbao b21quocbao added the type-bug Something isn't working label Jun 3, 2022
@tommasini tommasini added needs-triage Issues that require triage stability-team labels Jun 8, 2022
@Hmac512
Copy link

Hmac512 commented Jun 15, 2022

I can report the same issue is happening on iOS as well.

This bug is going to cause a lot of hurdles for our users for an upcoming NFT launch.

The issue only happens with array values.

{
  "types": {
    "EIP712Domain": [
      { "name": "name", "type": "string" },
      { "name": "version", "type": "string" },
      { "name": "chainId", "type": "uint256" },
      { "name": "verifyingContract", "type": "address" },
      { "name": "salt", "type": "bytes32" }
    ],
    "Mint": [
      { "name": "keys", "type": "string[]" },
      { "name": "values", "type": "string[]" }
    ]
  },
  "primaryType": "Mint",
  "domain": {
    "chainId": 80001,
    "name": "Test",
    "salt": "0x0000000000",
    "verifyingContract": "0x00000",
    "version": "1"
  },
  "message": {
    "keys": ["Make", "Model", "Year"],
    "values": ["Make", "Model", "Year"]
  }
}

Here is an example of a payload which fails on metamask, but is successful on Trust and Rainbow.

For this method over wallet connect:

eth_signTypedData_v4

Here is a video

@Hmac512
Copy link

Hmac512 commented Jun 16, 2022

I found another detail that may help the debug process.

Using the payload used in the dApp tester

https://github.com/MetaMask/test-dapp/blob/39a45cfb6096477a835eae059dc1a587a32c9c81/src/index.js#L1097-L1146

When using an injected provider (dApp browser for metamask mobile, and metamask browser plugin) there is no issue. It's able to sign the payload, and I can recover the address correctly.

However, when using the exact same payload as the dApp test over wallet connect with MetaMask mobile, it crashes the app, and is unable to sign.

@gantunesr
Copy link
Member

Thanks @b21quocbao and @Hmac512 for reporting, I'm taking a look at this issue and I'll give an update soon

@Hmac512
Copy link

Hmac512 commented Jun 22, 2022

@gantunesr We actually figured this out.

It turns out the bug is not with metamask, but with @walletconnect/web3-provider 1.7.8

For some reason, when I make a eth_signTypedData_v4 request, somewhere it gets clobbered to eth_signTypedData without a version.

Trust and Rainbow default to v4, while Metamask defaults to v3. V3 doesn’t support arrays, and so if you send a payload with an array value it crashes Metamask.

Perhaps it’s worth validating a request to make sure it won’t crash. It took me forever to figure out the issue.

Here is a patch that fixes the issue.

https://github.com/Hmac512/walletconnect-web3-provider-fix/blob/main/%40walletconnect%2Bweb3-provider%2B1.7.8.patch

@Fatxx Fatxx added Priority - High Task with high priority Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking and removed needs-triage Issues that require triage labels Jun 22, 2022
@gantunesr gantunesr removed Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking Priority - High Task with high priority labels Jun 22, 2022
@gantunesr
Copy link
Member

gantunesr commented Jun 22, 2022

Thanks for the update @Hmac512! A validation would help to avoid the crash, I'll open a PR to address it

@midgerate
Copy link

@gantunesr I created a PR.

@kevinwo
Copy link

kevinwo commented Sep 22, 2022

With the fix now in place in WalletConnect 1.8.0, is MetaMask able to take this update in the app, or does another blocker remain?

@gantunesr
Copy link
Member

Hey @kevinwo, thanks for pointing the new version of WC with the fix. We'll validate the new version with MetaMask and update this issue with the results.

@kevinwo
Copy link

kevinwo commented Sep 22, 2022

@gantunesr Sounds great, thank you!

@billyjacoby
Copy link

Are there any updates on this issue?

@Hmac512
Copy link

Hmac512 commented Oct 25, 2022

Are there any updates on this issue?

If you’re building the app that is having an issue, then you can fix this issue by using the patch above.

@billyjacoby
Copy link

Are there any updates on this issue?

If you’re building the app that is having an issue, then you can fix this issue by using the patch above.

Yeah unfortunately we need this to make it to the distributed version of the app in order for our users to benefit.

@Hmac512
Copy link

Hmac512 commented Oct 26, 2022

Yeah unfortunately we need this to make it to the distributed version of the app in order for our users to benefit.

The issue in this thread is not a bug in MetaMask, it is a bug in WalletConnect. This should be easy to fix within the app source code.

@kevinwo
Copy link

kevinwo commented Oct 26, 2022

For my use case, I found success by explicitly calling to the V4 method when making a request to MetaMask with WalletConnect. Instead of using eth_signTypedData, I use eth_signTypedData_v4.

@tommasini
Copy link
Contributor

If the desire it's to make a request with sign-typed data v4, you will need to make a custom request with wallet connect with the methodeth_signTypedData_v4
Example:
const customRequest = { jsonrpc: '2.0', method: 'eth_signTypedData_v4', params: msgParams, };
connector.sendCustomRequest(customRequest)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Something isn't working WalletConnect WalletConnect related issue or bug
Projects
None yet
Development

No branches or pull requests

9 participants