OKX security record

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)

All the instructions that update the protocol configurations forward their logic to the helper function `update_protocol_config_account`. This function modifies the data of the global protocol configuration (`ProtocolConfig` account). It can change the protocol's: - Status: if the status is `false`, the program is frozen, this means that all the marketplace instructions are not callable - Fee base points - Fee recipient - Manager The mentioned helper function checks if the caller is signing the transaction but it doesn't check if the caller is the `protocol_manager` saved on the global protocol configuration (`ProtocolConfig` account). This means that the instructions using this helper function can be called by anyone, not only by the `protocol_manager`.

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 okwallet-core component contains the AesEncrypt and AesDecrypt functions which use the encryption key that is hardcoded at `audit/okwallet-core/util/util.go:267`: ```GO var AesHardKey = common.MD5("1697773335922") // AesEncrypt AES 加密数据 func AesEncrypt(message, key string) (string, error) { if len(message) == 0 { return "", fmt.Errorf("message is empty") } if len(key) == 0 { key = AesHardKey } return common.Encrypt(message, key) } ``` No function calls towards these functions have been observed within the provided codebase. If this code snippet is not actively used, there are no security issues. **However, if this code is being used in other parts of the client’s software, this potentially poses a serious security problem.** ***Note:*** The reason this issue is undetermined is that we have only partial code. Although no reference to this hard coded secret was found in the code given to us, we can not conclude that code not shared with us did not use this hard-coded secret As such, the Severity is set as Informational, however it could be higher.

We identified a snippet of code that could allow an integer overflow if a sufficiently large integer is cast as a `uint32`. The snippet of code is found here: `bitcoin/brc20/util.go:15` ```Go func ConvertToUint32(v string) uint32 { i, _ := strconv.Atoi(v) return uint32(i) } ``` ***Note:*** This function is actively used in the codebase but it is not possible to determine if there is a real risk of an integer overflow in the client’s codebase, as we cannot establish the runtime values of the provided parameters. As such, the Severity is set as Informational, however it could be higher.