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

Bug: signTypedData_v4 fails when verifyingContract isn't a evm address #26980

Closed
foker opened this issue Sep 7, 2024 · 13 comments · Fixed by #27021
Closed

Bug: signTypedData_v4 fails when verifyingContract isn't a evm address #26980

foker opened this issue Sep 7, 2024 · 13 comments · Fixed by #27021
Assignees
Labels
external-contributor regression-prod-12.1.3 Regression bug that was found in production in release 12.1.3 release-12.5.0 Issue or pull request that will be included in release 12.5.0 release-12.6.0 Issue or pull request that will be included in release 12.6.0 Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-confirmations Push issues to confirmations team type-bug

Comments

@foker
Copy link

foker commented Sep 7, 2024

Describe the bug

We work with cosmos blockchain and some transactions in it need to be formed with signTypedData_v4
And now we can't for transaction if 'verifyingContract' field is not evm-like address (which is a common and valid situation for cosmos blockchain, with which Metamask works)

Problem with it appeared yesterday, after users got '12.1.3' update for metamask-extension
We checked the same behavior in 12.0.4 and it works correctly
I did a short investigation and it looks like it happened after updating version of @metamask/eth-json-rpc-middleware dependency to ^14.0.1.
Which, in turn, has broken functionality with next validation:

`
/**

  • Validates verifyingContract of typedSignMessage.
  • @param data - The data passed in typedSign request.
    */
    function validateVerifyingContract(data: string) {
    const { domain: { verifyingContract } = {} } = parseTypedMessage(data);
    if (verifyingContract && !isValidHexAddress(verifyingContract)) {
    throw rpcErrors.invalidInput();
    }
    }
    `

Expected behavior

eth_signTypedData_v4 method doesn't throw if verifyingContract isn't a evm-valid address

Screenshots/Recordings

Steps to reproduce

Working example :
(I took it from here ) :
await window.ethereum.request({ "method": "eth_signTypedData_v4", "params": [ "0xcb4dd4c1605117abc9bbcf1834377d7988d8e03b", { types: { EIP712Domain: [ { name: "name", type: "string" }, { name: "version", type: "string" }, { name: "chainId", type: "uint256" }, { name: "verifyingContract", type: "address" } ], Person: [ { name: "name", type: "string" }, { name: "wallet", type: "address" } ], Mail: [ { name: "from", type: "Person" }, { name: "to", type: "Person" }, { name: "contents", type: "string" } ] }, primaryType: "Mail", domain: { name: "Ether Mail", version: "1", chainId: 1, verifyingContract: "0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826" }, message: { from: { name: "Cow", wallet: "0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826" }, to: { name: "Bob", wallet: "0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB" }, contents: "Hello, Bob!" } } ], });

broken example
I changed verifyContract to cosmos:

await window.ethereum.request({ "method": "eth_signTypedData_v4", "params": [ "0xcb4dd4c1605117abc9bbcf1834377d7988d8e03b", { types: { EIP712Domain: [ { name: "name", type: "string" }, { name: "version", type: "string" }, { name: "chainId", type: "uint256" }, { name: "verifyingContract", type: "address" } ], Person: [ { name: "name", type: "string" }, { name: "wallet", type: "address" } ], Mail: [ { name: "from", type: "Person" }, { name: "to", type: "Person" }, { name: "contents", type: "string" } ] }, primaryType: "Mail", domain: { name: "Ether Mail", version: "1", chainId: 1, verifyingContract: "0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826" }, message: { from: { name: "Cow", wallet: "0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826" }, to: { name: "Bob", wallet: "0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB" }, contents: "Hello, Bob!" } } ], });

Error messages or log output

`buildRequest.js:35 Uncaught (in promise) InvalidInputRpcError: Missing or invalid parameters.
Double check you have provided the correct parameters.

Details: Invalid input.
Version: viem@2.9.4
    at delay.count.count (buildRequest.js:35:1)
    at async attemptRetry (withRetry.js:12:1)`

Detection stage

In production (default)

Version

12.1.3

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

Critical

@foker foker added the type-bug label Sep 7, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Sep 7, 2024
@metamaskbot metamaskbot added external-contributor regression-prod-12.1.3 Regression bug that was found in production in release 12.1.3 labels Sep 7, 2024
@randy75828
Copy link

Second to this! All cosmos chain using eip712 for metamask tx signing are not working because of this validation. Can someone from the team fix it?

@DanielTech21 DanielTech21 added Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-confirmations-system DEPRECATED: please use "team-confirmations" label instead labels Sep 9, 2024
@DanielTech21
Copy link

Hi @foker

Thank you for bringing this to our attention.

We will review the issue and get back to you.

@cryptotavares cryptotavares added team-confirmations Push issues to confirmations team and removed team-confirmations-system DEPRECATED: please use "team-confirmations" label instead labels Sep 9, 2024
@digiwand
Copy link
Contributor

digiwand commented Sep 9, 2024

greetings @foker,

Thanks for raising this issue! The working and broken examples look the same and work.

That said, I think I see what you mean by the provided code you shared, and it appears to be a regression.

We are investigating this further. Are you able to provide the address you used for the broken verifyingContract example as it might help with the investigation?

@bschorchit
Copy link

@foker @randy75828 please do share a broken example once you have a chance. We're investigating how we can revert this regression for cosmos without reverting the changes for evm addresses.

@foker
Copy link
Author

foker commented Sep 9, 2024

Can you please try like that

await window.ethereum.request({ "method": "eth_signTypedData_v4", "params": [ "0xcb4dd4c1605117abc9bbcf1834377d7988d8e03b", { types: { EIP712Domain: [ { name: "name", type: "string" }, { name: "version", type: "string" }, { name: "chainId", type: "uint256" }, { name: "verifyingContract", type: "address" } ], Person: [ { name: "name", type: "string" }, { name: "wallet", type: "address" } ], Mail: [ { name: "from", type: "Person" }, { name: "to", type: "Person" }, { name: "contents", type: "string" } ] }, primaryType: "Mail", domain: { name: "Ether Mail", version: "1", chainId: 1, verifyingContract: "cosmos" }, message: { from: { name: "Cow", wallet: "0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826" }, to: { name: "Bob", wallet: "0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB" }, contents: "Hello, Bob!" } } ], });

My address is 0xcB4dd4C1605117aBc9bBcf1834377d7988d8E03b

@danjm
Copy link
Contributor

danjm commented Sep 10, 2024

@foker @randy75828 @mtsitrin Is there any documentation anywhere on why cosmos dapps use the verifyingContract field in this way?

@randy75828
Copy link

randy75828 commented Sep 10, 2024

https://github.com/evmos/ethermint/blob/main/ethereum/eip712/domain.go#L29

we integrate this library when we want to extend evm capability for cosmos chains. Even though this repo is read-only now, i believe most cosmos chains still uses this.

As far as I know, there is no documentation as to why it is cosmos. I presume it is just to satisfy the interface and therefore the library provided a dummy string for verifyingContract

@mtsitrin
Copy link

I've created a PR that supposed to fix this issue (untested in prod)
MetaMask/eth-json-rpc-middleware#333

@jpuri jpuri self-assigned this Sep 11, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 11, 2024
danjm pushed a commit that referenced this issue Sep 11, 2024
… verifyContract field validation for cosmos (#27065)

Cherry-picks #27021
(4ee09fc)

## **Description**

Adding patch on eth-json-rpc-middleware to disable verifyContract field
validation for cosmos

## **Related issues**

Fixes: #26980

## **Manual testing steps**

1. Submit a types signature request with verifyingContract set to
`cosmos`
2. Ensure that you are able to sign it

## **Screenshots/Recordings**
<img width="360" alt="Screenshot 2024-09-10 at 4 08 00 PM"
src="https://github.com/user-attachments/assets/a56bb8bd-abed-4fae-9c50-a3a25addd5b5">

## **Pre-merge author checklist**

- [X] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [X] I've completed the PR template to the best of my ability
- [X] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [X] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@danjm
Copy link
Contributor

danjm commented Sep 11, 2024

@randy75828 @mtsitrin @foker We are preparing a release that should fix this issue. It applies a very similar solution to @mtsitrin's proposed PR

Would one of you be able to download a release candidate build and test that it fixes your dapp? We would like to be certain that it fixes the problem. To do so:

  1. View this github comment and click "builds ready": Version v12.2.3 RC #27066 (comment)
  2. Download the chrome build (the first in the list, next two "builds:")
  3. Open chrome://extensions
  4. Toggle on "developer mode"
  5. Toggle off your existing metamask installation
  6. Drag and drop the downloaded zip file onto the chrome://extensions page
  7. Open this metamask install, onboard, and test the functionality
  8. Once you have completed testing, remove the metamask install, toggle on your other metamask install, and toggle off "developer mode"

Alternatively, if you are unable to do that, could one of you give us step by step instructions on how we can test this on a live dapp?

@mtsitrin
Copy link

Hi @danjm
Just tested it. It works.
Tested over https://portal.dymension.xyz

Thx for the quick response

@danjm
Copy link
Contributor

danjm commented Sep 11, 2024

thank-you @mtsitrin!

@foker
Copy link
Author

foker commented Sep 12, 2024

we tested on our side too, works like a charm

@metamaskbot
Copy link
Collaborator

Missing release label release-12.5.0 on issue. Adding release label release-12.5.0 on issue, as issue is linked to PR #27021 which has this release label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor regression-prod-12.1.3 Regression bug that was found in production in release 12.1.3 release-12.5.0 Issue or pull request that will be included in release 12.5.0 release-12.6.0 Issue or pull request that will be included in release 12.6.0 Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-confirmations Push issues to confirmations team type-bug
Projects
Archived in project
10 participants