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

Different signTypedData results between 5.1.0 and 6.0.1 #340

Closed
vvvvvv1vvvvvv opened this issue Oct 16, 2023 · 2 comments · Fixed by #354
Closed

Different signTypedData results between 5.1.0 and 6.0.1 #340

vvvvvv1vvvvvv opened this issue Oct 16, 2023 · 2 comments · Fixed by #354

Comments

@vvvvvv1vvvvvv
Copy link

Typed Message

{
  types: {
    SafeTx: [
      { type: "address", name: "to" },
      { type: "uint256", name: "value" },
      { type: "bytes", name: "data" },
      { type: "uint8", name: "operation" },
      { type: "uint256", name: "safeTxGas" },
      { type: "uint256", name: "baseGas" },
      { type: "uint256", name: "gasPrice" },
      { type: "address", name: "gasToken" },
      { type: "address", name: "refundReceiver" },
      { type: "uint256", name: "nonce" },
    ],
    EIP712Domain: [
      { name: "chainId", type: "uint256" },
      { name: "verifyingContract", type: "address" },
    ],
  },
  domain: {
    chainId: "1",
    verifyingContract: "0xb023c11fbcbd0f8d02c85a7cf51a1eab085e6d67",
  },
  primaryType: "SafeTx",
  message: {
    to: "0xf5d5b661f9c3d8b5244f00ef25cf25b61f5a1f7d",
    value: "100000000000000",
    data: "0x",
    operation: "0",
    safeTxGas: "0",
    baseGas: "0",
    gasPrice: "0",
    gasToken: "0x0000000000000000000000000000000000000000",
    refundReceiver: "0x0000000000000000000000000000000000000000",
    nonce: "18",
  },
}

Private Key(empty test address)

7d0ebc17b57137adc429b3f51bf5803fa61e3cd1050e9520d05ed5bcafbcff18

Code example

const utils = require("@metamask/eth-sig-util");

const data = {
  types: {
    SafeTx: [
      { type: "address", name: "to" },
      { type: "uint256", name: "value" },
      { type: "bytes", name: "data" },
      { type: "uint8", name: "operation" },
      { type: "uint256", name: "safeTxGas" },
      { type: "uint256", name: "baseGas" },
      { type: "uint256", name: "gasPrice" },
      { type: "address", name: "gasToken" },
      { type: "address", name: "refundReceiver" },
      { type: "uint256", name: "nonce" },
    ],
    EIP712Domain: [
      { name: "chainId", type: "uint256" },
      { name: "verifyingContract", type: "address" },
    ],
  },
  domain: {
    chainId: "1",
    verifyingContract: "0xb023c11fbcbd0f8d02c85a7cf51a1eab085e6d67",
  },
  primaryType: "SafeTx",
  message: {
    to: "0xf5d5b661f9c3d8b5244f00ef25cf25b61f5a1f7d",
    value: "100000000000000",
    data: "0x",
    operation: "0",
    safeTxGas: "0",
    baseGas: "0",
    gasPrice: "0",
    gasToken: "0x0000000000000000000000000000000000000000",
    refundReceiver: "0x0000000000000000000000000000000000000000",
    nonce: "18",
  },
};

const pk = Buffer.from(
  "7d0ebc17b57137adc429b3f51bf5803fa61e3cd1050e9520d05ed5bcafbcff18",
  "hex"
);

const sig = utils.signTypedData({
  privateKey: pk,
  data,
  version: utils.SignTypedDataVersion.V4,
});

console.log("sig", sig);

Result

in v5.1.0, result is 0xb0e34cee18e7ded5fda0cc248f540e003b676a6a24336121df89ead770357334072f6fb61ceb4d840755a497fa8623e0a7404342f7eee7d439bbdaa5c86fbd321c

in versions later than v6.0.1, result is 0xc197219c9b3b41df23f8d52cb5afe2208437f07527c29fbd49af6665f0ef752c1c17c85f758e625bf09e973bf6122e9794687d7e35e03b6ffff04bed973c3b5b1b

@vvvvvv1vvvvvv vvvvvv1vvvvvv changed the title Different results between 5.1.0 and 6.0.1 Different signTypedData results between 5.1.0 and 6.0.1 Oct 16, 2023
@legobeat
Copy link
Contributor

legobeat commented Nov 20, 2023

@vvvvvv1vvvvvv Thank you for reporting the issue and your patience.

A fix is in #354, which will make your test-case yield the same result as in v5.1.0.

Pending releases:

Gudahtt added a commit to MetaMask/metamask-mobile that referenced this issue Nov 20, 2023
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 resovled by patching `@metamask/keyring-controller` to
delegate signing typed messages to the keyrings. The very next release
of that package includes that change anyway. We will wait until this
bug is fixed before updating `@metamask/eth-sig-util` in the keyrings.

Fixes #7792
Gudahtt added a commit to MetaMask/metamask-mobile that referenced this issue Nov 20, 2023
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 patching `@metamask/keyring-controller` to
delegate signing typed messages to the keyrings. The very next release
of that package includes that change anyway. We will wait until this
bug is fixed before updating `@metamask/eth-sig-util` in the keyrings.

Fixes #7792
Gudahtt added a commit to MetaMask/metamask-mobile that referenced this issue Nov 21, 2023
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 patching `@metamask/keyring-controller` to
delegate signing typed messages to the keyrings. The very next release
of that package includes that change anyway. We will wait until this
bug is fixed before updating `@metamask/eth-sig-util` in the keyrings.

Fixes #7792
@legobeat
Copy link
Contributor

Fix is now available in:

Gudahtt added a commit to MetaMask/metamask-mobile that referenced this issue Nov 29, 2023
## **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**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [x] 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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] 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.

---------

Co-authored-by: legobt <6wbvkn0j@anonaddy.me>
danroc added a commit to MetaMask/snap-simple-keyring that referenced this issue Dec 12, 2023
A regression was introduced on eth-sig-util 6.0.1 and 7.0.0, see:
MetaMask/eth-sig-util#340
danroc added a commit to MetaMask/snap-simple-keyring that referenced this issue Dec 12, 2023
A regression was introduced on eth-sig-util 6.0.1 and 7.0.0, see:
MetaMask/eth-sig-util#340
sethkfman pushed a commit to MetaMask/metamask-mobile that referenced this issue Dec 22, 2023
## **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**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [x] 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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] 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.

---------

Co-authored-by: legobt <6wbvkn0j@anonaddy.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants