OKX (Threshold-lib)

In the Schnorr proof implementation, the computation of the challenge value `h` doesn't match with the specification. According to the specification, the challenge c is computed as `H(G || V || A)`, where `H` is a hash function, `G` is the base point of the elliptic curve, `V = rG` is the random commitment, and `A` is Alice's public key. However, in the implementation, the challenge `h` is computed as `crypto.SHA256Int(X.X, X.Y, R.X, R.Y)`, which is a hash of the `X` and `Y` coordinates of the public key `X` and the random commitment `R`. The base point `G` is not included in the computation. This discrepancy between the implementation and the specification can lead to potential vulnerabilities or incorrect results when using the Schnorr proof. The implementation needs to be corrected to follow the specification.

The generation of random numbers in a crypto safe way plays a paramount role in ensuring the security of the implemented cryptographic algorithms. Such task can be delegated to the underlying operating system which offer such service using a mixture of hardware and software mechanism to generate unpredictable and unique random values. In particular, the codebase in scope always relies on the standard `rand.Reader` utility which, calling the operating system, generates random strings of bytes. However, there are some cases in which users of crypto wallets require strong guarantees of randomness for some reasons: the runtime environment may not be fully trusted, several source of randomness may be required, an external dedicated hardware for random number generation may be available, and so on. Moreover, given the sensibility of the implemented algorithms, a common testing practice is to compare the implementation with the same algorithm executed in a different programming language. Since many algorithms rely on random numbers and results are not deterministic, in order to perform such tests, deterministic random number generators are put in place so the final results and intermediate outcomes can be compared. Since the codebase in scope implements cryptographic algorithm built **on top** of a secure random number generator, the randomness security is not a responsibility of the library and hard-coding it to the default random byte generator of Go may be limiting for the library usage.

The [BIP-32](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki) specification document adopts HMAC-SHA512 for generating derived child data from parent information. Such algorithm provides data authentication through an hash function and a key. It takes two parameters, an authentication key and the data to authenticate. The BIP-32 specification uses what it calls chaincode as authentication key, and the parent public key concatenated with the derivation index as data to authenticate. However the derivation function implemented at the pointed location uses a constant hard-coded string as authentication key and prepends the chaincode to the data to authenticate. Even though such changes do not introduce security problems thanks to the hash function properties, the BIP-32 definitions are not respected, so the library can not claim to be compliant and interoperable with correct implementations of such standard.

The cryptography algorithms implemented in the codebase in scope strictly follow their reference paper by wrapping each step or round of a party participating in the protocols in dedicated methods of a single struct. The context of each session of an algorithm is kept by storing information in the assigned method struct, while messages coming from the interacting counterparties are passed as methods parameters. By following this pattern it is important that struct methods are executed according to the prescribed order so that the struct state is consistent with the algorithm progress in a certain session. Any wrong method call would invalidate the state, cause an information loss and force the algorithm session abort. In order to avoid such situation, the structs wrapping the context for Distributed Key Generation with Verified Secret Sharing, Key Reshare and Ed25519 signature include a `RoundNumber` field which explicitly track the algorithm progress. Then, all the methods implementing algorithm steps check if the current `RoundNumber` is the one supposed to be in the invoked round before executing the assigned logic and update the `RoundNumber`, as well. However, such verification mechanism of the current round is absent in the ECDSA signature implementation based on [Lidell17](https://eprint.iacr.org/2017/552.pdf). In this way, the correctness of methods call order is completely delegated to the library caller, which may not be aware of such constraints or may unwillingly implement a faulty logic.

The `hash.Cmp(C) == 0` code used in commitment verification is not a constant comparison procedure which could potentially lead to timing side channel attack. Although the auditors did not notice it is exploitable at this moment but it is suggested to perform constant timing comparison.

In function `NewRefresh` and `NewSetUp` of this library, the developers check parameter `total` to unsure the number of participated parties is no less than 2. However, the code simply panics if the constrain does not meet. As a library which can be used as part of code in other developers' project, the auditors suggest that use error message instead of panic to show that the passed parameters are inappropriate, which is consistent with the rest of the library coding style.

The audited Go project is designated to run with version 1.17. However, Go language (before version 1.19.5) suffers an issue ([GO-2023-1621](https://pkg.go.dev/vuln/GO-2023-1621)) where the ScalarMult and ScalarBaseMult methods of the P256 Curve may return an incorrect result if called with some specific unreduced scalars. Although this does not directly impact the audited project for now, this may impact future version of the audited library. Similarly, Go language runtime (version 1.17) also suffers an issue ([GO-2023-1840](https://pkg.go.dev/vuln/GO-2023-1840)) on Unix platform which could lead to information leakage or privilege escalation. Although the audited code is used as library instead of full binary, the users of this library may keep the same Go version thus auditor still consider this as potential impact.

The aim of the key reshare process is to make a change in the set of shareholders while keeping the same global key-pair. This is achieved through the participation of two shareholders from the old set which, relying on their shares, act as dealers of new shares for the new set of shareholders. Such dealers are called `devotees` in the codebase while non-devotees simply verify the incoming information in order to assess that the process was concluded successfully. In the current implementation, non-devotees generate a local secret, a random polynomial to share such secret, a Schnorr proof to demonstrate the knowledge of the secret and the verifiers of the polynomial coefficient. Such data are also forwarded in the messages of step 1 and 2. However, all these computations are not necessary since the only source of information is represented by the devotees, and only their messages will be actually verified and taken into account in step 3 when finalizing the reshare algorithm.

The `s` variable in the `bigIntToEncodedBytes` function is initialized as a reference to a 32 byte array through the `new` operator which also allocates memory for the specified type. However, the memory allocated at variable initialization is practically unused since the a new `byte` array is allocated by the `copyBytes` function and the reference is replaced.