DerivaDEX 2 security record

The code loops through the `registeredCustodians` of a specific release hash in a few places: `Custodian.sol:L185-196` (the `setMinimumBond` function), `Registration.sol:L221-231` (the `updateReleaseSchedule` function), and `LibRelease.sol:L54-65` (the `evaluateReleaseSchedule` function). Too many custodians/signers registered for a specific release hash can block the for-loop by running out of gas. Among the potential failures, the failure on the `LibRelease:evaluateReleaseSchedule` function is the most severe one. The failure of the `LibRelease:evaluateReleaseSchedule` function will cause the function `Checkpoint.sol:checkpoint` to fail. Therefore, the checkpoint can fail to proceed if too many registered custodians exist.

If, during an upgrade, `diamondCut(...)` calls are executed in multiple Ethereum transactions, users may be exposed to contracts that are upgraded only partially, i.e., some of the functions are upgraded while others are not. This may result in unexpected inconsistencies.

The function `LibCollateral.sol:generateTraderGroupKey` (L630-638) simply keep the "0" as the chain discriminator. According to the code document, the design preserves 0 for the Ethereum chain. There are some risks with this discriminator. First, one should not deploy this code to another chain. So future code changes and audits should be aware of this. Meanwhile, the "Ethereum" chain can refer to test nets and the main net. The current implementation will fail to discriminate the test net from the main net.

DerivaDEX, as currently implemented, relies heavily on off-chain components. For example, a minimum number of two registered custodians is required at all times for the proper functioning of the protocol. However, if custodians are hacked or DDoSed, this may cause problems for the protocol.

From the reference sparse Merkle tree documentation ([link](https://justjjy.com/An-optimized-compact-sparse-merkle-tree)), it states that: "Since the key is included in the leaf’s hash, and the key is unique in SMT, no matter what the value is, the leaf_hash(key, value) is unique". However, in the function `LibSMT.sol:hashLeaf` (L225-236), it directly returns `0x00` if the leaf's value is empty. This `0x00` value violates the spec that the leaf hash should be unique no matter the value. One can give two different keys with an empty value. The "leaf-hash" returned from the function will both be empty. Luckily, this vulnerability does not impact any of the current contracts. The `LibSMT.sol:computeRoot` function is used inside `Collateral.sol`, `FundedInsuranceFund.sol`, and `Stake.sol`. All of the current occurrences provide a non-empty leaf value.

The function `initialize(...)` does not have an explicit guard against being called more than once. The current implementation relies on the operation to carefully remove the `initialize` from the `functionSelectors` when calling `diamondCut`. This can be prone to error, and the impact can be huge once the facet is re-initialized.

Solidity 0.8 embeds the overflow/underflow check as part of the compiler. Thus, it is no longer needed or recommended to use the `SafeMath` library. (Note that this issue occurs after the re-audit fix for QSP-18)

The `LibSMT.sol:computeRoot` (L63-84) accepts an array of leaves and a proof to calculate the root. However, all the code referencing the `LibSMT.sol:computeRoot` only passes the `_leaves` with a single element in the array (see: `Collateral.sol:L370`, `FundedInsuranceFund.sol:L136`, and `Stake.sol:L185`). As a result, it is possible to change the function signature of the `LibSMT.sol:computeRoot` to accept single leaf input. If `LibSMT.sol` only needs to support the computation of a single leaf, it can remove the function of `LibSMT.sol:operationHashSiblings`. Furthermore, the need for the stack diminishes as the computation only needs to stack a single node (or two items: one key and one value) inside the stack at a time. The code can replace the stack with two variables.