DerivaDEX 2

Quantstamp22 Jul 2022Files named in the report

Audit

Quantstamp reviewed DerivaDEX 2 on 22 Jul 2022: 23 findings, 0 critical; 12 marked fixed, 8 acknowledged, 3 open.

No open critical findings

Open the report Open DerivaDEX 2’s security record

Report hosted on certificate.quantstamp.com.

Trustblock does not review code.

Findings23

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	0	2
Medium	1	1	1
Low	2	7	9

MediumGas Concern: Looping Through `registeredCustodians`Acknowledged

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.

LowDiamond Facet UpgradeAcknowledged

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.

LowHardcoded Chain DiscriminatorAcknowledged

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.

LowHighly Dependent on Off-Chain ComponentsAcknowledged

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.

LowNon-Unique SMT Leaf Hash on Empty ValueAcknowledged

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.

LowRisk of `initialize(...)` Called More than OnceAcknowledged

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.

LowUnnecessary `SafeMath`Acknowledged

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)

LowUnnecessary Complexity of `LibSMT`Acknowledged

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.