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.

Sign up for GitHub

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

Jump to bottom

calling personal_sign with a hex string results in garbled text #5473

Closed
ghost opened this issue Oct 9, 2018 · 4 comments
Closed

calling personal_sign with a hex string results in garbled text #5473

ghost opened this issue Oct 9, 2018 · 4 comments

Comments

@ghost
Copy link

ghost commented Oct 9, 2018 •
edited by ghost
Loading

Describe the bug
Calling the personal_sign RPC method with a hex string causes the MetaMask signature confirmation popup window to display garbled text, because it interprets the string as UTF8.

To Reproduce

  1. Create a simple HTML page and serve it locally:
<html>
  <head>
    <script>
      window.addEventListener('load', async () => {
        web3.currentProvider.sendAsync({
          method: 'personal_sign',
          params: ["0x9c22ff5f21f0b81b113e63f7db6da94fedef11b2119b4088b89664fb9a3cb658", <YOUR_ADDRESS_HERE>],
          from: <YOUR_ADDRESS_HERE>
        }, (error, result) => {
          if (error || result.error) console.error(error)
          console.log(result.result)
        })
      })
    </script>
  </head>
</html>
  1. Observe the garbled text.

Expected behavior
As far as I can tell, the message argument passed to personal_sign is interpreted by MetaMask as a UTF8-encoded string cast to hex (e.g., to sign 'test', one would pass '0x' + Buffer.from('test', 'utf8').toString('hex') i.e. 0x74657374).

My expectation is that 32-byte hex strings passed to personal_sign, e.g. 0x9c22ff5f21f0b81b113e63f7db6da94fedef11b2119b4088b89664fb9a3cb658 should be interpreted as such, and displayed accordingly in the signature window.

Note that despite displaying the incorrect text, the resulting signature is correct, and can be verified using Solidity's ecrecover and code like the following:

bytes memory prefix = "\x19Ethereum Signed Message:\n32";
bytes32 prefixedMessageHash = keccak256(abi.encodePacked(prefix, messageHash));
return ecrecover(prefixedMessageHash, v, r, s) == _address;

Screenshots
image

Browser details (please complete the following information):
Chrome: Version 69.0.3497.100 (Official Build) (64-bit)
OS: macOS High Sierra 10.13.6 (17G65)
MetaMask: 4.13.0
UI: New/Beta
@ghost
Copy link
Author

ghost commented Dec 3, 2018

@bitpshr when you get a chance would you mind checking this out? It's an easy repro/clear bug, and is significantly affecting UX on a MetaMask site I'm working on :)

@bdresser
Copy link
Contributor

bdresser commented Dec 3, 2018

@NoahHydro this has been an open issue for a while - see #3931 for some background.

Since we now support signTypedData, I'd encourage that as an alternative

@NoahZinsmeister NoahZinsmeister mentioned this issue Dec 3, 2018
Merged
@NoahZinsmeister
Copy link
Contributor

@bdresser I'm aware of signTypedData, but my preference in smart contract development is to use ERC191 v0 signatures.

@bdresser
Copy link
Contributor

bdresser commented Dec 3, 2018

Totally fair. Thanks for the PR!

@ghost ghost closed this as completed Feb 7, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bdresser @NoahZinsmeister