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 a bug of addition law of elliptic curve groups #81

Closed
wants to merge 8 commits into from

Conversation

cychuang0924
Copy link

@cychuang0924 cychuang0924 commented Dec 12, 2019

I notice that the origin addition law has a bug.
https://github.com/binance-chain/tss-lib/blob/883f207b38f0357d7a8c7fa957301c7a6f9b0848/crypto/ecpoint.go#L50

For example:
Let N be the order of an elliptic curve group and G is a generator of the elliptic curve group. Then
if you try to use "Add" to compute 2*G+(N-2)*G, then the output is an error.

This phenomenon will cause the following implementation which has possible failure.
https://github.com/binance-chain/tss-lib/blob/883f207b38f0357d7a8c7fa957301c7a6f9b0848/ecdsa/keygen/round_3.go#L135

If you meet the case 2*G+(N-2)*G, then culprits is not empty. However, the case 2G+ (N-2)G
should be handled.

Following your nice codes, we recommend that

  1. Denote X()=nil and Y()=nil to be identity element in the elliptic curve group (i.e. aG+(nil, nil) = aG, for any a in [0,N-1]), which also implies that this point X()=nil and Y()=nil is on the curve. You can imaginary this point intercepting the elliptic curve at infinity.

  2. Add function should handle some special cases. After this modification, this "Add" can
    sum any two elements in elliptic curve groups including the following cases:

  • 0G+aG =aG or aG+0G =aG
  • aG+(N-2)G = 0G
  • 0G + 0G = 0G
  • aG + aG = 2aG (i.e. The original function supports this case.)

If you don't like my coding style, then you can rewrite it.

Well, some functions maybe also need to modify. For examples:

"FlattenECPoints", UnFlattenECPoints and so on.....

Thank you for your patience.

@ZhAnGeek
Copy link
Contributor

@cychuang0924 Hi, thanks for submit fixes, could you fix the tests so we could merge?

@ZhAnGeek ZhAnGeek closed this Jan 12, 2024
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.

3 participants