search

serai-dex / serai

Other
266 stars 49 forks source link

fix CLSAG verification. #552

Closed Boog900 closed 7 months ago

Boog900 commented 7 months ago

We were not setting c1 to the last calculated c during verification, instead keeping it set to the one provided in the signature, allowing forgeries.

@kayabaNerve I did message you on matrix but it seems there are still issues so I decided just to open this PR, although this is bad AFAIK no one relies on this ATM.

kayabaNerve commented 7 months ago

No. This is a critical in not only monero-serai yet in the multisig functionality specifically. This would cause invalid participants in the FROST process to not be flagged as it'd think the signature is still (wrongly) valid.

On the one hand, monero-serai wasn't audited (queued), I haven't done my final pass, and it's not a 1.0. On the other, I'm fucking pissed this made it through and horrified. At best, I can be proud monero-serai did enough right it caught enough other people this was itself caught.

Matrix is acting up and I don't have any messages from you. Please reach out on Discord (Serai Discord to find me) or Telegram (kayabaNerve), or let me know if I need to make a new Matrix account on monero.social so we can sync on this.

kayabaNerve commented 7 months ago

All versions of monero-serai are currently yanked for an independent reason (several months ago). I never re-published after (I don't even think I've published the audited bitcoin-serai), so no yanking needed due to this. Still outrageous.

kayabaNerve commented 7 months ago

Expansion of basic tests, as even those would've caught this (further emphasizing the fault here), fuzz tests, and prioritizing https://github.com/serai-dex/serai/issues/549? That's my immediate thoughts on the matter. Possibly addition of code coverage evaluators? That'd highlight such missing cases...

kayabaNerve commented 7 months ago

Also, to be perfectly clear, only myself is to blame here. All my frustration is self-directed.

Boog900 commented 7 months ago

I don't use any of those other platforms. If the Matrix issues are causing problems in other chats, it might be best to make a monero.social account otherwise I could make a matrix.org account.

Boog900 commented 7 months ago

+1 for more tests

Boog900 commented 7 months ago

I ended up making a matrix.org account: boog900:matrix.org