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

eth/signature: fix prefix_message for multibyte characters #120

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

kudohamu
Copy link
Contributor

Hi, thanks for the useful gem!

I encountered a problem when verifying signature signed with web3.js that in some cases could not be verified correctly.
So I tried signing messages using web3.js and eth.rb, and noticed that different signatures are generated only if they contain multibyte characters.

Here is an example.

message signature (generated by web3.js) signature (generated by eth.rb)
hello, ethereum! 0x7b84abfdddd62aee9c598b98f74ae17b7b497b5530a92bd8f262ac33bccaed230fce76c74609e1c8c96b1bebe1a62e46477d3327481d8ed674d6c51bb98c689c1b 0x7b84abfdddd62aee9c598b98f74ae17b7b497b5530a92bd8f262ac33bccaed230fce76c74609e1c8c96b1bebe1a62e46477d3327481d8ed674d6c51bb98c689c1b
こんにちは、イーサリアム! 0xdd809dedbf2099c2306bb443ae201e24fc7fd7fd4f0c00a961b5bae0decc74d715b4715cab5010157e6662b8abcad0062eb47b63cce986623d43a884eb9ff2e41c 0xf502a9db634e0b1e5ccbb88ffe934855c5211b072bcf4b5b51084086c804c61b35b8e29780071685673d8bd3fe2df1f9ee5b569ba13f9363afc2833b0f66215a1b

What's happening?

I found a difference in the logic of prefixing with "known messages" in web3.js and eth.rb.
In the known message, eth.rb counts the number of characters, but web3.js counts the number of bytes.

Which is correct?

The EIP-191 specification quotes go-ethereum as follows.

"\x19Ethereum Signed Message:\n" + len(message).

len, a built-in function of golang, is designed to "returns the number of bytes" in the case of string.
ref: https://pkg.go.dev/builtin@go1.18.3#len

Therefore, I decided that it was a mistake on the eth.rb side and created this PR.

@kurotaky
Copy link
Collaborator

Thanks for the PR :) I will review it.

@kurotaky kurotaky self-requested a review June 21, 2022 14:53
@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2022

Codecov Report

Merging #120 (98b8993) into main (bb640c7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #120   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files          67       67           
  Lines        3855     3858    +3     
=======================================
+ Hits         3845     3848    +3     
  Misses         10       10           
Impacted Files Coverage Δ
lib/eth/signature.rb 100.00% <100.00%> (ø)
spec/eth/signature_spec.rb 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Collaborator

@kurotaky kurotaky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kurotaky kurotaky merged commit 7cba13b into q9f:main Jun 24, 2022
@q9f q9f added the bug Something isn't working label Jun 27, 2022
@kudohamu kudohamu deleted the fix-prefix-message branch June 28, 2022 00:16
mculp pushed a commit to mculp/eth.rb that referenced this pull request Aug 1, 2022
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

Successfully merging this pull request may close these issues.

4 participants