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

Fix comparison while creating signatures #11

Merged

Conversation

radhe-zeeve
Copy link
Contributor

@radhe-zeeve radhe-zeeve commented Aug 19, 2023

Issue

When performing the string based comparison for two hexadecimal values (in this case while forming the signature for a requested sequence with data committee member private key), the logic does not consider the fact that the Bytes2Hex function returns output in lowercase char. for the hexadecimal string.

While performing comparison with 7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0 using the lower case string it gives wrong results for most of the cases. This is because the ASCII values of upper case and lower case char. are different, which is what is considered while performing the string based comparison.

Example

LHS (hash): 7a9d59c0b984259000bfbb7787965552e85f6b84f8a85461fc8fc050cc47ab41
RHS: 7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0

if strings.ToUpper("7a9d59c0b984259000bfbb7787965552e85f6b84f8a85461fc8fc050cc47ab41") > "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0" {} // false

if "7a9d59c0b984259000bfbb7787965552e85f6b84f8a85461fc8fc050cc47ab41" > "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0" {} // true

This leads to creating unacceptable signatures (creating wrong s component within v,r,s signature) which doesn't get verified by the "data committee" smart contract (when invoked by supernets2 smart contract).

Following is the error thrown by supernets2-sequence-sender service, right after collecting signatures from data committee service, while estimating the gas before performing tx. on L1

2023-08-19T10:11:43.200Z	ERROR	ethtxmanager/ethtxmanager.go:80	failed to estimate gas: execution reverted: ECDSA: invalid signature 's' value, data: 438a539900000000000000000000000000000000000000000000000000000000000000600000000000000000000000004d119cea58e8bde28f653dc0ac9058e924e74e050000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000151303168c59fdbfcc2848e24860510e32d226419699d086a64f366f994390a6b9294e626dad921aad25a7fba963992f5b09a6b9e20cd1a8c3e523dd157d5b6ae0000000000000000000000000000000000000000000000000000000064e08d400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005553b9a88da168fe9e77b32409ef1477a965cab70d9f522990eccaa9e88650a627839d36d22f7303651ac8e4d7fe5986ffc1e6200571a1415a17c060704353b4341ba58eec45e33bd5c1e06e4a7a58869c75ddaa01ed0000000000000000000000	{"pid": 1, "version": "v0.2.0-RC1-26-ga5f96d97"}
github.com/0xPolygonHermez/zkevm-node/ethtxmanager.(*Client).Add
	/src/ethtxmanager/ethtxmanager.go:80
github.com/0xPolygonHermez/zkevm-node/sequencesender.(*SequenceSender).tryToSendSequence
	/src/sequencesender/sequencesender.go:141
github.com/0xPolygonHermez/zkevm-node/sequencesender.(*SequenceSender).Start
	/src/sequencesender/sequencesender.go:60

Solution

There are two possible solutions

  1. Convert the string to upper case on output of Bytes2Hex before comparing the string.
  2. To load both hexadecimal values in BigInt before comparison.

We have applied the first solution while was the one going for.

Signed-off-by: Jasti Sri Radhe Shyam <radheshyam.jasti@zeeve.io>
@christophercampbell christophercampbell merged commit 8365568 into 0xPolygon:main Aug 21, 2023
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 this pull request may close these issues.

2 participants