search

oxen-io / eth-sn-contracts

4 stars 8 forks source link

Concerns Regarding Potential Algebraic Attacks on Low-Level Cryptographic Implementations in Libraries/*.sol #11

Closed venezuela01 closed 3 months ago

venezuela01 commented 8 months ago

I'm concerned about the security of libraries/BN256G1.sol, libraries/BN256G2.sol, and libraries/Pairing.sol, considering:

  1. BN256G2.sol originates from a third-party open-source implementation that:
    • has not seen industry usage
    • no updates for 5 years
    • containing hundreds of lines of low-level elliptic curve arithmetic operations.
  2. BN256G1.sol and Pairing.sol also haven't been tested in any industry use case.

These contracts are set to bear the responsibility for assets worth tens of millions of dollars. If there are any security vulnerabilities in this code, Oxen/Session investors will be the first to suffer losses, as no one else has tested these libraries in the past.

There's a well-known lesson: Don't roll your own cryptography (unless you are an expert). I wonder how many developers on the Oxen team fully understand every single line of these low-level mathematical operations and their consequences.

I was once shocked to realize that some core developers at Oxen have made rookie mistakes by confusing point multiplication inverse and scalar multiplication inverse when devising homemade cryptography. I hate to say this, but I couldn't convince myself a team that made such an entry-level mistake could grasp every single line of these cryptographic arithmetic implementation. Perhaps you used to understand these mathematical concepts during school, but if you're not working with them daily, there are countless opportunities for mistakes.

Can you please clarify the security of these libraries? What efforts have been made to ensure their security? How confident are you in these libraries? What level of security do you aim to achieve before deploying to production with real-world assets? How many people on the team have successfully implemented an algebraic attack against an elliptic curve cryptography implementation, even just as a school assignment? If you don't know how to attack another's library, how can you know how to defend your own? How much do you know about the state of the art in attacking and defending these cryptographic algorithms?

While third-party security auditing is valuable, it must be conducted by the right experts. Smart contract security specialists are not necessarily experts in cryptographic mathematics, and you need individuals skilled in both smart contract security and cryptographic math security.

Please forgive me for being direct. I don't mean to be aggressive or offensive; I'm just very concerned.

KeeJef commented 8 months ago

These libraries will be audited by external smart contract auditors and cryptographers before this goes into production, rest assured this is a major area of focus for us as well, as we don't have the expertise in the team to audit these contracts ourselves.

venezuela01 commented 8 months ago

These libraries will be audited by external smart contract auditors and cryptographers before this goes into production, rest assured this is a major area of focus for us as well, as we don't have the expertise in the team to audit these contracts ourselves.

Glad to hear that. Thanks for the hard work. If there is anything we can help with, feel free to shout out.

darcys22 commented 8 months ago

Whats your issue with the inversion functions?

venezuela01 commented 8 months ago

Whats your issue with the inversion functions?

If you're referring to 'confusing point multiplication inverse and scalar multiplication inverse when devising homemade cryptography', then that's a separated issue unrelated to smart contracts. It has been reported and patched but not yet released, I can't disclose the details publicly until a bug fix version is released. Kee and Jason are aware of it, or you can DM ons venezuela, and I can share it with you.

(Apologies for any rudeness and lack of clarity in my original post.)

darcys22 commented 8 months ago

Thanks for clarifying, yeah the initial post reads like you had found something wrong with this codebase.

venezuela01 commented 3 months ago

The recent audit by the Zellic team and the follow-up fix have largely reduced concerns about low-level crypto bugs in the implementation.

Glad that they found something serious like: Zellic 3.3: Incorrect curve mapping, follow up and Zellic 3.4: Bias in hashToField function

Thanks for the work. I'm closing this issue and placing my trust in the professional audit experts.

https://github.com/oxen-io/eth-sn-contracts/pull/41