status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.9k stars 987 forks source link

personal_sign method does not work in status #11333

Closed bgits closed 3 years ago

bgits commented 4 years ago

Bug Report

Using the personal_sign method does not seem to work within Status while it works from web3 wallets.

Problem

Using the personal sign method fails to trigger a populated dialog that can be signed by the user.

An overview of the background required to understand the problem.

Attempting to use snapshot.page we discovered that Status is the only wallet unable to sign messages for voting.

A problem description.

Using the following test page: https://danfinlay.github.io/js-eth-personal-sign-examples/

When clicking on personal_sign it should trigger a populated dialog to sign (as in metamask). It either fails to trigger or populated an empty dialog box as when attempting to vote using snapshot.

Expected behavior

It should populate a dialog box and allow the user to sign it.

Actual behavior

Either no dialog box appears or an empty one sign request appears.

Reproduction

Additional Information

...
flexsurfer commented 4 years ago

so there is the error ReferenceError: Can't find variable: web3 , status doesn't inject web3 library , it's not related , personal_sign works in status

bgits commented 4 years ago

Another way to recreate the error is using snapshot which is the primary use case that is being blocked by this issue now. It does trigger the signing box but it is empty.

You can recreate this error by going to:

https://demo.snapshot.page/#/status/create

and attempting to publish a poll by filling in all the fields.

flexsurfer commented 4 years ago

@bgits it seems you're trying to sign typed data ? in that case you have to use a different method eth_signTypedData_v3

bgits commented 4 years ago

Here is another test: trying to sign both a plain text string and typed data works using metamask but neither works in Status.

https://gateway.pinata.cloud/ipfs/QmckPsNLbGSQjTTAwchqyrUrG7FwnrhdHsJh5n7gMn9ykV/

flexsurfer commented 4 years ago

in this example sign message doesn't work because it uses eth.sign which is not supported, and for typed data it uses eth_signTypedData_v4 and status doesn't support it, we could add support for it if needed

flexsurfer commented 4 years ago

do you know what's the difference between v3 and v4 and why you can't use v3 ?

bgits commented 4 years ago

do you know what's the difference between v3 and v4 and why you can't use v3 ?

It's being used because that is what ethers.js exposes. It does not exposes any other typed signer.

bgits commented 4 years ago

@bgits it seems you're trying to sign typed data ? in that case you have to use a different method eth_signTypedData_v3

I don't believe the error is due to typed data. The data being sent over is a JSON to sign but not typed data. Furthermore I created an additional test to use the same personal_sign method with a string and it produces the same error in Status.

updated test can be found here: https://gateway.pinata.cloud/ipfs/QmXECbBSchgQja9VQAqQopNTMwtwKcF46BEWrCgkfTJYGN/

flexsurfer commented 4 years ago

we expect the message as a hex for personal_sign method

flexsurfer commented 4 years ago

eth_signTypedData_v4 https://github.com/status-im/status-go/issues/2062

bgits commented 4 years ago

we expect the message as a hex for personal_sign method

This is problematic because the user will not be able to understand what they are signing. How would signing status principles work for example?

This is also not consistent with how other wallets handle it.

flexsurfer commented 4 years ago

This is problematic because the user will not be able to understand what they are signing.

we convert it to a string, all dapps sign fine in status, CK and others

This is also not consistent with how other wallets handle it.

it works according to spec in status https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign

bgits commented 4 years ago

it works according to spec in status https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign

I don't see anything in the spec that requires the message to be in hex. This is consistent with how every other wallet is handling it.

bgits commented 4 years ago

all dapps sign fine in status,

This is not true. This issue is open because https://snapshot.page/ does not sign in status while it does work with other wallets.

This is the most used voting dapp and we have been trying to use it with Status and are blocked by this.

flexsurfer commented 4 years ago

then we need to implement something similar to https://github.com/MetaMask/metamask-extension/pull/1177

bgits commented 4 years ago

we convert it to a string,

It also does not display hex as a string to the user. https://gateway.pinata.cloud/ipfs/QmVjDxwxTN4zDPDp9NzzANEEp5pUhNzKGUjHXFD9Sf1MRw/

flexsurfer commented 4 years ago

It also does not display hex as a string to the user. https://gateway.pinata.cloud/ipfs/QmVjDxwxTN4zDPDp9NzzANEEp5pUhNzKGUjHXFD9Sf1MRw/

0x5445535420535452494e47000000000000000000000000000000000000000000, length: 66, text : TEST STRING

let's ask @richard-ramos about that https://github.com/status-im/status-react/commit/277f65b94d563a981d780490a20a4802f03f82ca

flexsurfer commented 4 years ago

it looks like its just will be better to implement it similar to MM

flexsurfer commented 4 years ago

thanks @bgits

richard-ramos commented 4 years ago

let's ask @richard-ramos about that 277f65b

I added that because back when I was working on teller network, the GSN made the users sign a random bytes32 hash that wasn't being shown at all in status, or displayed weird characters, and this way it would show the hex string (or at least did back then)

...

@bgits, how do you trigger the personal_sign to show the ascii for the messages when the lenght of the message is 32 bytes? If I execute this on metamask i get the following result:

web3.eth.personal.sign("0x54455354204d4553534147450000000000000000000000000000000000000000", web3.eth.defaultAccount)

image

bgits commented 4 years ago

@bgits, how do you trigger the personal_sign to show the ascii for the messages when the lenght of the message is 32 bytes?

@richard-ramos https://github.com/status-im/ethers-testbed/blob/master/src/App.jsx#L68-L70

cammellos commented 4 years ago

Regarding signtypeddata_v4 , I have noticed that metamask might not be according to specs:

https://github.com/MetaMask/eth-sig-util/issues/106

https://github.com/MetaMask/eth-sig-util/pull/107

So before we proceed any further it would be good to clarify exactly which one is the correct behavior.

We will also look at personal_sign.

flexsurfer commented 4 years ago

same for personal_sign , there is no spec for signing utf8 strings, metamask just did their own implementation

richard-ramos commented 4 years ago

On desktop for personal_sign what I did was check if the input value is a hex string, and if it's not, convert it to hex before signing