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

Changing 'contents' field of a Poll Option Doesn't Change Signature #17

Closed
apbendi opened this issue Apr 13, 2020 · 5 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@apbendi
Copy link
Contributor

apbendi commented Apr 13, 2020

Go to the Create Poll page and enter a bunch of data. Sign it, and observe the signature in the console. Change the text of one poll option, but don't change anything else about the submission. Re-submit and re-sign the data, observing the signature in the console. It will be the same. Bizarre!

@apbendi
Copy link
Contributor Author

apbendi commented Apr 13, 2020

By the way, this happens on the backend as well. Is it a bug in the signing library? Are we misunderstanding how to use it? I'm very confused by it.

@mds1
Copy link
Contributor

mds1 commented Apr 13, 2020

Whoa, this is really weird. Good catch.

It looks like this only happens with the poll options and not for any other fields—did you find the same thing?

I tried all 4 combinations of (1) poll_options array as array of objects vs. array of strings, and (2) poll_options type in dataFormat of string vs. bytes32. But all combinations resulted in the same signature even after changing a poll option and confirming that different data is shown in MetaMask...

One workaround would be to include something random/variable in the pollData object sent to the server. For example, the start time can instead be determined by the frontend and validated on the backend. Alternatively, the frontend can generate some random salt to include as part of the pollData object. This would force different signatures to be generated every time, even if the options and other poll data are identical.

But I feel like we should probably dig a bit more to figure out why this is happening.

@mds1
Copy link
Contributor

mds1 commented Apr 13, 2020

So one thing that seems relevant is there is a newer method called eth_signTypedData_v4, whereas we are currently using eth_signTypedData_v3 to get the signature. Apparently, eth_signTypedData_v4 is supposed to add support for arrays (1, 2, 3), but when I change a type in the dataFormat from, say, bytes32 to bytes32[], MetaMask complains about an invalid type

@apbendi
Copy link
Contributor Author

apbendi commented Apr 13, 2020

I'm super confused now, because I thought the whole point of creating that stringified digest was because we were actually just signing the string. But apparently not? Unclear to me what we're actually signing.

when I change a type in the dataFormat from, say, bytes32 to bytes32[], MetaMask complains about an invalid type

What's the exact error message. Might try grepping the MetaMask codebase at this point to see where it's being thrown from and what types they consider permissable.

@mds1
Copy link
Contributor

mds1 commented Apr 13, 2020

I'm super confused now, because I thought the whole point of creating that stringified digest was because we were actually just signing the string. But apparently not? Unclear to me what we're actually signing.

Yea, I am equally confused about this. My only guess is that it strips out unsupported types, e.g. arrays of some format. But they should be supported with eth_signTypedData_v4. So if you're testing this on the frontend, make sure to change eth_signTypedData_v3 to eth_signTypedData_v4 and I'll fix it in the branch I'm working on

What's the exact error message. Might try grepping the MetaMask codebase at this point to see where it's being thrown from and what types they consider permissable.

If using eth_signTypedData_v3:

image

If using eth_signTypedData_v4. This was generated by changing

{ name: 'poll_options', type: 'string' },

to

{ name: 'poll_options', type: 'string[]' },

Same error message is thrown if using bytes32[] instead

image

@apbendi apbendi assigned apbendi and unassigned mds1 Apr 22, 2020
@apbendi apbendi assigned mds1 and unassigned apbendi May 4, 2020
@mds1 mds1 mentioned this issue May 5, 2020
7 tasks
@apbendi apbendi closed this as completed in 7e8aec1 May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants