fix: Fix eth_signTypedData signatures containing 0x

[Gudahtt]

Description

In v7.10.0, signatures including a bytes field with the value 0x were being encoded differently than before. Previously the string 0x was interpreted as an "empty" hex number, but as of v7.10.0 they were encoded as the string "0x". This change was not intentional, and it resulted in invalid signatures.

This problem was introduced in @metamask/eth-sig-util@6.0.1 (see here for details: MetaMask/eth-sig-util#340). This package was introduced to mobile when @metamask/keyring-controller was updated to v6 (#7267). The keyrings themselves still use @metamask/eth-sig-util@5.0.1, but the KeyringController is signing these messages directly rather than delegating to the keyrings, which is why the newer @metamask/eth-sig-util package is used here.

This has been resolved by updating all affected versions of @metamask/eth-sig-util.

Related issues

Fixes #7792

Manual testing steps

1. Navigate to https://gudahtt.github.io/test-dapp/ in the in-app browser
2. Sign a message using eth_signTypedData_v4
3. Attempt to verify that message. On production (v7.10.0) this does not work. It should work correctly on this branch, returning the signing account address.

Screenshots/Recordings

Before

https://recordit.co/xejXpo4mlh

After

https://recordit.co/NnnpxbCVRB

Pre-merge author checklist

• I’ve followed MetaMask Coding Standards.
• I've clearly explained what problem this PR is solving and how it is solved.
• I've linked related issues
• I've included manual testing steps
• I've included screenshots/recordings if applicable
• I’ve included tests if applicable
• I’ve documented my code using JSDoc format if applicable
• I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
• I’ve properly set the pull request status:
  • In case it's not yet "ready for review", I've set it to "draft".
  • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (13acaa7) 37.44% compared to head (a835b4f) 37.44%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7886   +/-   ##
=======================================
  Coverage   37.44%   37.44%           
=======================================
  Files        1052     1052           
  Lines       28193    28193           
  Branches     2517     2517           
=======================================
  Hits        10557    10557           
  Misses      17037    17037           
  Partials      599      599           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[legobeat]

FYI, pending release proposals for actually fixing the issue in @metamask/eth-sig-util:

• 6.0.2 eth-sig-util#357
• 7.0.1 eth-sig-util#355

[legobeat]

We will wait until this bug is fixed before updating @metamask/eth-sig-util in the keyrings.

Alternatively/additionally, #7902 should resolve the issue by replacing both affected versions with fixed updates.

[github-actions]

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

[socket-security]

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@metamask/test-dapp	7.2.0...8.0.0	None	+0/-0	4.45 MB	gudahtt

[socket-security]

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: @metamask/eth-sig-util@6.0.2

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

[Gudahtt]

Thanks @legobeat ! That does look like a simpler solution than a patch. I've stolen that commit from your PR and paired it with a test-dapp bump, so that we can consolidate the e2e test coverage and test evidence in one place.

[Gudahtt]

@SocketSecurity ignore @metamask/eth-sig-util@6.0.2

lgbot is a trusted maintainer

[Gudahtt]

This would be ready for review, except that attempts to test this are blocked by #7920

The e2e regression tests are failing for that same reason

[Gudahtt]

Passing e2e smoke test run: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/5b720d88-8e03-4a03-aa55-e55452fa0bb2

Passing e2e regression test run: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/36bd1108-3d0e-46b2-8273-bd0b46849cfc

[github-actions]

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/aabbb8b7-bda6-4046-af19-c34f754644fa
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[cortisiko]

@Gudahtt this PR is ✅

In addition to what the E2E tests covered, I also tested signing transactions using keystone.

Additionally I went ahead and did a quick smoke test on one of the dapps used to reproduce the issue.

Scenario:
connect to dapp via WC
proceed to sign a txn.
upon doing so I no longer can reproduce the bug:

iOS: http://recordit.co/aPTiJWrDDq
Android: http://recordit.co/Xpu37j5zVI

Compared to prod where the bug is reproducible: http://recordit.co/UjZ4hqsjHy

[gauthierpetetin]

Hi @Gudahtt , do you think this issue is related?
#7913 (comment)

[metamaskbot]

No release label at all on PR. Adding release label release-7.12.5 on PR, as PR was cherry-picked in branch 7.12.5.