OKX (Threshold-lib)

The audited version of the library was found to be vulnerable to the Lindell17 Abort Vulnerability (CVE-2023-33242), where an attacker could potentially extract the full private key from a wallet implementing the Lindell17 2PC protocol. This vulnerability arises from deviations in the Lindell17 implementations from the specification of the academic paper, particularly in handling aborts during failed signature attempts. It was discovered that an attacker, with privileged access, could exploit this vulnerability to exfiltrate the key after approximately 200 malicious signature requests, thereby posing a severe security risk to the affected systems and their users. The root cause of the Lindell17 Abort Vulnerability stems from some implementations of the Lindell17 protocol mishandling or ignoring aborts in cases of failed signatures. This oversight allows an attacker, assuming privileged access, to exploit this flaw and extract a full private key by initiating malicious signature requests, posing a significant security threat to the affected systems. Vulnerability detail: [https://www.fireblocks.com/blog/lindell17-abort-vulnerability-technical-report](https://www.fireblocks.com/blog/lindell17-abort-vulnerability-technical-report)

In the mentioned code snippet, the developers use the go routine function `crypto.GenerateSafePrime` to generate two safe prime numbers. The `crypto.GenerateSafePrime` function takes `quit` channel parameter as a control signal to terminate the prime generation process. Using code from `paillier.go` as an example: ```go var values = make(chan *big.Int) var quit = make(chan int) var p, q *big.Int for p == q { for i := 0; i < currency; i++ { go crypto.GenerateSafePrime(PrimeBits/2, values, quit) } p, q = <-values, <-values close(quit) } ``` The above code creates a channel `quit` before launching go routines. The for loop checks if generated two prime numbers are the same. The loop continues if two same prime numbers are acquired. However, the channel `quit` is closed in every iteration of the for loop, meaning if the loop continues then the repeated `close` action on the already closed `quit` channel will cause unexpected `panic`.

The `for` constructs at the pointed locations initializes the `sharePubKeyMap` map with the public keys obtained from the secret shares commitments sent by the parties of the protocol. Such data are then used to verify the correctness of the owned share. The `map` initialization is stopped after $t+1$ iterations, where $t$ is the threshold to overcome to reconstruct the global shared secret. However, $n$ can be greater than $t+1$ as the number of parties participating in the protocol is not related to $t$. In this way, when $n > t+1$, the parties with ID higher than $t+1$ can not verify the correctness of their share and the DKG protocol is interrupted. The pointed locations refer to the correspondent steps the key generation and reshare, which both present the same incorrect behavior.

The key generation sub-protocol in [Lindell17](https://eprint.iacr.org/2017/552) ends up with the two parties having the global public key, their local private key share. Moreover, the entity playing party 2 also obtains a Paillier encryption of party 1 private share that will be necessary in the signing sub-protocol. In order to convince party 2 that the shared encryption actually is the chipertext corresponding to the local private share behind the shared partial public key, party 1 also sends to party 2 a proof demonstrating such assertion. A method to compute and verify such proof is reported in section 6 of [Lindell17](https://eprint.iacr.org/2017/552). In the scenario implemented in the codebase in scope, parties generate key shares using a combination of multiple (one per party) VSS sessions. Then, in order to run the Lindell protocols, the Paillier encryption of the local share is shared between the signing actors (the $c_{key}$ value). However, no proof that such encryption is correct is shared by party 1, so party 2 can only trust that the shared information is correct. In the case in which such encryption is not correct, party 1 can induce party 2 either in computing useless data or in participating in the calculation of a signature verifiable by a random key-pair that only party 1 is aware of.

Using code from `paillier.go` as example: ```go var values = make(chan *big.Int) var quit = make(chan int) var p, q *big.Int for p == q { for i := 0; i < currency; i++ { go crypto.GenerateSafePrime(PrimeBits/2, values, quit) } p, q = <-values, <-values close(quit) } ``` This code launches currency number of go routines in each iteration of the loop, each running the GenerateSafePrime function. However, only two values (i.e., `p` and `q`) are read from the values channel in each iteration. This means that if currency is greater than 2, there will always be some go routines whose results are not read in each iteration. These go routines will be blocked waiting for their results to be read, leading to a go routine leak. This leak can cause unnecessary resource consumption and potential performance degradation in the program.

In the following code, the developers did not check if curve is existed in curve map. In Go programming language, the value will be `nil` or `0` if key is not found in the map. ```go func GetCurveByName(curveName string) elliptic.Curve { return curveMap[curveName] } ``` This function is further used in the following code, which takes unsanitized JSON input and retrieve the curve by JSON `curve` field. ```go func (p *ECPoint) UnmarshalJSON(payload []byte) error { aux := &struct { Curve string X *big.Int Y *big.Int }{} if err := json.Unmarshal(payload, &aux); err != nil { return err } p.X = aux.X p.Y = aux.Y p.Curve = GetCurveByName(aux.Curve) if !p.IsOnCurve() { return fmt.Errorf("UnmarshalJSON error, point not on the curves ") } return nil } ``` However, the code will panic (caused by null pointer dereference) if curve's name is not precisely `secp256k1` or `ed25519`.

The current `DlnProof` implementation is largely borrowed from Binance open source threshold signature library. The corresponding code is located at [https://github.com/bnb-chain/tss-lib/blob/master/crypto/dlnproof/proof.go](https://github.com/bnb-chain/tss-lib/blob/master/crypto/dlnproof/proof.go). However, the adapted code reduced the number of components of the proof from the original `128` to `12`. This reduction in iterations weakens the security of the proof. In fact, the number of iterations in an interactive zk-proof (even though made non-interactive with the Fiat-Shamir heuristic) directly impacts the probability that the proof correctly demonstrates its assertion. A reduced number of iterations may make possible for an attacker to issue a proof which verifies as `true` while the underlying fact to demonstrate is `false`. While `12` iterations may speed up the computation, they do not represent a big enough value to trust the outcome of the proof verification, thereby potentially compromising the security of the system.

The `math/big` library is always included at the end on the import list, while according to the `gofmt` formatting, it should be placed with the Go standard library before external dependencies. Also, external dependencies should go at the end of the import list.

Random number generation plays a critical role in the threshold library. There are two main random number generation functions used in this library. 1. `getRandom` in `paillier.go` for generating a random number `r` which is `r < n` and `gcd(r,n) = 1`. ```go func getRandom(n *big.Int) (*big.Int, error) { gcd := new(big.Int) r := new(big.Int) var err error for gcd.Cmp(one) != 0 { r, err = rand.Int(rand.Reader, n) if err != nil { return nil, err } gcd = new(big.Int).GCD(nil, nil, r, n) } return r, nil } ``` 2. `RandomNum` in `utils.go` which generate a random number `r` which is `1 < r < n`. ```go func RandomNum(n *big.Int) *big.Int { if n == nil { panic(fmt.Errorf("RandomNum error, n is nil")) } for { r, err := rand.Int(rand.Reader, n) if err != nil { panic(fmt.Errorf("RandomNum error")) } if r.Cmp(one) == 1 { return r } } } ``` There are several inconsistent coding issues presented in the above code. 1. `getRandom` does not check against `n == 0` where `RandomNum` does. 2. `getRandom` returns error message where `RandomNum` simply panics if are any random number generation issue. 3. Both function do not check if `n` is negative value.

The address of cited paper regarding `NewPDLwSlackProof` design is invalid. Accessing to `https://www.cs.unc.edu/~reiter/papers/2004/IJIS.pdf` is no longer valid.