dlog smallgroup 8 - Githubissues

bnb-chain / tss-lib

Threshold Signature Scheme, for ECDSA and EDDSA

MIT License

790 stars 271 forks source link

dlog smallgroup 8 #115

Closed ackratos closed 3 years ago

omershlo commented 3 years ago

let's call 8inv8 taking a point and multiply it by eight followed by multiplication by inverse of eight. Ideally, 8inv8 should be part of deserialise operation so in the communication layer. Otherwise - it should be right after a point is being constructed on the receiver side and before the point is being used in any computation:

for keygen / reshare - I would do 8inv8 right after https://github.com/binance-chain/tss-lib/blob/master/eddsa/keygen/round_3.go#L80
for signing - right after https://github.com/binance-chain/tss-lib/blob/master/eddsa/signing/round_3.go#L52

0xmountaintop commented 3 years ago

Is this PR to make sure cofactor being 8, in order to avoid small subgroup attacks?

omershlo commented 3 years ago

@HAOYUatHZ yes

omershlo commented 3 years ago

@ackratos is PR ready or still WIP?

ackratos commented 3 years ago

@ackratos is PR ready or still WIP?

WIP, signing test failed. I will make some time tomorrow to see what's going wrong

ackratos commented 3 years ago

@ackratos is PR ready or still WIP?

@omershlo its wired that some point's coordinate changed after 8mod8:

before: (33552789945003341559556071573290394328859640448630510168930667598912185992237,36929837543392076051436230083844271994203432191481006648631453933492208242121)
after: (1241292298444314692362221258714346540404859950536562280771500437945613275563,30443200966919248487730911485609075598047291289395336389722104605321905523418)

Do you have any clue about what's going wrong?

omershlo commented 3 years ago

That shouldn't happen, I tested locally as well and for any point on the curve 8mul8 should return the same point.

Can you share a code snippet or point me to where in the code you get this issue?

ackratos commented 3 years ago

That shouldn't happen, I tested locally as well and for any point on the curve 8mul8 should return the same point.

Can you share a code snippet or point me to where in the code you get this issue?

you can just run TestE2EConcurrentAndSaveFixtures inside eddsa/keygen in this branch https://github.com/binance-chain/tss-lib/pull/118/files

omershlo commented 3 years ago

I found your issue,

you declare eight and eightInv as var in ecpoint.go. The problem is that eightInv is computed with the default tss.EC() which is set to secp256k1 while what you need is for tss.EC() to be set to Edwards beforehand. What happens is that you get a mix between the two curves (because tss.EC() is set to Edwards during the protocol) . To solve it - you should compute the eightInv with the Edwards curve order