DerivaDEX 2 security record

The `Checkpoint.sol:checkpoint` function will first jail the minority submitters before verifying the consensus rule. Those jailed signers will decrease the `dsRegistration.validSignersCount` counted in the `LibCheckpoint.sol:getConsensusThreshold` function. The ability to jail others brings a potential attack where the smaller group of signers tries to jail the larger group of singers. And then, since the minority group jail the counter-parties, they can easily pass the validation as long as the smaller group of signers exceeds the `quorum` requirement.

The function `Reject.sol:reject` (L56-126) checks whether there is consensus failure or not with the checkpoint `_submissions` input data. The calculation is based on the number of `custodians` in L96-97 retrieved from the `_submissions[i]`. However, the `reject` function does not check whether it counts the specific custodian multiple times. Thus, a single signer can fake several submissions and pretend that there are several groups.

The `MathHelpers.sol:safe128` function casts the input `n` to `uint96` instead of `uint128`. This will return an over-casted value if the `n` is in the range of `[2^96, 2^128)`. The `safe128` function is used in `block.number` calculations and also the function `LibCollateral.sol:scaleAmountForToken`. It is unlikely the `block.number` will exceed `uint96`, but `LibCollateral.sol:scaleAmountForToken` function is of risk. Inside the `LibCollateral.sol:scaleAmountForToken` function, it will apply the `safe128` to `10^(abs(decimals - OPERATOR_DECIMALS))`. Although most ERC20s use practical decimals like 18, it is not impossible to have an ERC20 token with decimal more than usual. If the are more than 47 decimals, the calculation of `10^(48-18)` will exceed `2^96-1` and be over-casted after the `safe128` function. The `LibCollateral.sol:scaleAmountForToken` function is used inside the `Collateral.sol` and `FundedInsuranceFund.sol` contracts. It would be risky when depositing and withdrawing tokens if the DEX were to list any tokens with abnormal decimals

Several facets inherit the `ReentrancyGuard.sol` contract from the OpenZeppelin. However, the `ReentrancyGuard.sol` implementation does not integrate with the diamond storage. The `ReentrancyGuard.sol` will mutate a `_status` variable on the storage ([code](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/de99bccbfd4ecd19d7369d01b070aa72c64423c9/contracts/utils/ReentrancyGuard.sol#L36)). This brings risk to storage collision across facets if new facets access to the standard storage instead of the randomized diamond storage. Note that this is the only storage detected not following the diamond storage pattern within the current code base, so there is no exploit from this now. However, future code changes and audits should be aware of this risk.

DerivaDEX inherits from a Diamond Standard implementation of EIP-2535. However, it is unclear which repository and which commit hash the files are cloned from.

The `LibStack.sol` contract strongly assumes that the implementation should allocate no memory within the stack's life cycle. If a new memory is assigned, the stack will not fail and will continue to read from those newly issued memories. Note that the team already documented this limitation inside the code. Also, we did not find an exploit from this during the audit. Still, we would like to highlight this, so future code changes or audits should be cautious.

A few places in the code ignore the return value of the generic ERC20 transfer call. The implementation of the `transfer` and `transferFrom` functions of the ERC20 tokens can potentially return false to indicate a failed transfer instead of reverting directly. Therefore, a failed transfer might sneak through the code without checking the return value. 1. `Collateral.sol:L388` (the function `withdraw`): return value of `IERC20(...).transfer(...)` is not checked. 2. `FundedInsuranceFund.sol:L81` (the function `fund`): return value of `IERC20(_collateralAddress).transferFrom(...)` is not checked. 3. `FundedInsuranceFund.sol::L152` (the function `unfund`): return value of `IERC20(...).transfer(...)` is not checked.

The `LibCollateral.sol:commitWithdrawalAllowance` function (L251-275) has the condition of `rateLimitRemaining != uint128(-1)` in L269. However, if we look into the `LibCollateral.sol:getMaximumWithdrawal` function (L358-398), `rateLimitRemaining` is defined as `rateLimits.rateLimitForPeriod - rateLimits.consumedInPeriod` in L391. To fulfill the condition of `rateLimitRemaining != uint128(-1)`, the `rateLimits.consumedInPeriod` must be exactly `rateLimits.rateLimitForPeriod + 1` so that it can underflow and become `uint128(-1)`. There are some issues from our observation. First, the condition indicates the expectation and usage of the underflow. The use of underflow adds complexity to understanding the code and is against best practices. Aside from that, the calculation of `rateLimits.rateLimitForPeriod - rateLimits.consumedInPeriod` seems not possible to go underflow. The reason is that the `rateLimits.consumedInPeriod` is the sum of the `_unscaledWithdrawalAmount`. And the `_unscaledWithdrawalAmount` is guarded by the `maximumWithdrawalAmount` in L261 inside the function of `LibCollateral.sol:commitWithdrawalAllowance`. `maximumWithdrawalAmount` ensures that it will not be greater than the `rateLimits.rateLimitForPeriod`.

DerivaDEX uses Version 0.6.12 of Solidity, whereas the latest release of Solidity is 0.8.15. Several breaking changes removing unused/unsafe features have been implemented in the intervening versions. Moreover, an antiquated version makes it impossible to use the latest version of Diamond standard files.

Codebase contains several `TODO` and `FIXME` comments. These should not appear in a production codebase.