ZcashFoundation/redjubjub

Frost to merge

oxarbitrage opened this issue · 1 comments

The audit actions are of 2 types: Security and Others. This ticket is to keep track of the "Others". On each point we need a PR or an explication on why we consider no de change is needed.

Audit can be found in https://github.com/ZcashFoundation/redjubjub/blob/main/zcash-frost-audit-report-20210323.pdf

3.1 Potential API improvements

  • Restrict the maximum number of signers and threshold parameters. At the moment these parameters are defined using u32 type. Consider using u8 or to put some hard-coded constant for the maximal intended numbers.

    Comments: We are actually using u8 now but this might change.

  • Document what the library does in relation to how to import secret keys. This can be required in certain use cases. The trusted dealer will have a way to instead of randomly picking the secret key, to take existing one. - #104

  • Derive Debug , Eq , PartialEq for participants local public keys. This will allow implementers more freedom to work with public outputs of the key generation.

  • Serialization and deserialization may be implemented for messages. It is hard to imagine how the code can be run over distributed network without such functionality.

    Comments: Work in progress, the design is #85 and an initial implementation PR is at #95

Unused values

  • The KeyPackage structure is never used, which leads to the warnings.

    Potential fix at #99

3.2 Outdated dependencies

  • The byteorder and rand_core crates are outdated versions (presumably, rand_core is on purpose pre-0.6). We suggest to update to the latest version after testing it, and potentially reviewing the changes.

    core_rand updated at #55 and byteorder in #100

3.3 Test coverage

  • The amount of unit testing should be extended to better cover different success and failure scenarios, and generally better code coverage. For example, we recommend tests for various number of participants and thresholds, valid and invalid ones, and for different preprocessing scenarios.

3.4 unwrap() risks

  • We recommend to be careful with unwrap() usage, as it could lead to panics upon None/Err . Instead, use pattern matching to gracefully fail.

    There is a PR with some work in this subject at #101

3.5 Null ID support

  • When performing Shamir secret sharing, a polynomial f(x) is used to generate each party’s share of the secret. The actual secret is f(0) and the party with ID i will be given a share with value f(i) . Since a DKG may be implemented in the future, we recommend that the ID 0 be declared invalid.

3.6 Typo in a comment

As far as importing secret keys, we decided as a team that implementations can handle this functionality on their own. We do allow for serialization/deserialization of key material, which allows them to/restore save key material to/from disk. We might want to document this for users in the library documentation, though.