# Dtrade Audit Notes

**Auditor:** Jake Bunce

**Client:** Dtrade https://dtrade.org/

https://github.com/dtradeorg/dtrade_v1

**Files Scoped:** `contracts/TradeInfo.sol`, `contracts/TradeMining.sol`

https://github.com/dtradeorg/orderbook-contracts

**Files Scoped:** Repo

## Whitepaper & specification about the project

Whitepaper:

Project is a trading platform.

## Review of the protocol/implementation

**[1] Unlocked Pragma**

**Files Affected:** `contracts/TradeInfo.sol`, `contracts/TradeMining.sol`

**Severity: Low**

Default AL text.

**Recommendations:**

Default AL text.

**[2] Multiple Instances of Unsafe Arithmetic**

**Files Affected:** `contracts/TradeMining.sol`

**Severity: Medium**

[`_updateParamsForEpoch`](https://github.com/dtradeorg/dtrade_v1/blob/main/contracts/TradeMining.sol#L499) has multiple instances of arithmetic where operations are susceptible to over/underflows due to a lack of use of the `SafeMath` library.

**Recommendations:**

Either use the `SafeMath` library consistently throughout the project or move to Solidiy >= `0.8.0` which has this protection built into the compiler.

**[3] Inconsistent Variable Naming**

**Files Affected:** `contracts/TradeMining.sol`

**Severity: Informational**

The docstrings for [`variable assignments`](https://github.com/dtradeorg/dtrade_v1/blob/main/contracts/TradeMining.sol#L84) appear to use cycle and epoch interchangably.

**Recommendations:**

Use a consistent naming scheme or clarify the difference between an epoch and a cycle.

**[4] Expiry of Rewards**

**Files Affected:** `contracts/TradeMining.sol`

**Severity: Informational**

Users should be made aware of the expiry of rewards should they not be claimed.

**Recommendations:**

Provide comprehensive user documentation that describes this behaviour.

**[5] Constructor Argument Checks**

**Files Affected:** `contracts/TradeMining.sol`

**Severity: Low**

[`initialize()`](https://github.com/dtradeorg/dtrade_v1/blob/main/contracts/TradeMining.sol#L124) does not check if the address passed is a contract or an EOA for the reward token nor the trade info contract. This helps verify that the constructor arguments are correct.

**Recommendations:**

Use OpenZeppelin's [`Address.isContract()`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol) library to check if the address passed is a contract.

**[6] Unsafe Transfer Call**

**Files Affected:** `contracts/TradeMining.sol`

**Severity: Medium**

[`removeFunds()`](https://github.com/dtradeorg/dtrade_v1/blob/main/contracts/TradeMining.sol#L420) uses `transfer()` to send the DET token. This is unsafe as it uses a fixed amount of gas. Should the amount of gas required to execute increase in the future, this function may not complete.

**Recommendations:**

Use `call()` instead.

**[7] ABIEncoderV2 Not Considered Experimental**

**Files Affected:** `contracts/TradeInfo.sol`

**Severity: Low**

In Solidity version >=`0.7.4` the [`ABIEncoderV2`](https://github.com/dtradeorg/dtrade_v1/blob/main/contracts/TradeInfo.sol#L3) is not considered experimental.

**Recommendations:**

If the version of Solidity in use is >=`0.7.4` change the statement to `pragma abicoder v2`

**[8] Orderbook Solidity Version**

**Files Affected:** `https://github.com/dtradeorg/orderbook-contracts/*.sol`

**Severity: Informational**

`pragma solidity 0.5.16;` is the main version used in the orderbook repo. This is an old version and doesn't include protection for issues like over/underflows.

**Recommendations:**

Move to a more recent version, such as `=>0.8.0+`

**[9] No Validation of TradeInfo Contract**

**Files Affected:** `protocol/v1/impl/Admin.sol`

**Severity: Low**

[`setTradeInfo()`](https://github.com/dtradeorg/orderbook-contracts/blob/qs-audits/contracts/protocol/v1/impl/Admin.sol#L110) has no validation check to verify that the `tradeInfo` is the correct contract. Other update functions have this validation step. Same applies to [`setFeePool`](https://github.com/dtradeorg/orderbook-contracts/blob/qs-audits/contracts/protocol/v1/impl/Admin.sol#L121).

**Recommendations:**

Consider adding a check to verify the contract argument address is correct to be consistent with other functions.

**[10] Shared Event Emitted**

**Files Affected:** `protocol/v1/impl/MarginBank.sol`

**Severity: Low**

[`addMarginFromBank()`](https://github.com/dtradeorg/orderbook-contracts/blob/qs-audits/contracts/protocol/v1/impl/MarginBank.sol#L107) uses the `LogBankWithdraw` event, which is also used by `withdrawFromBank()` and `transferFrom()`. Each event should be unique so that it is clear what operation was taken when monitoring in production.

**Recommendations:**

Create seperate events and emit for each distinct operation.

**[11] Inconsistent Global Operator Management**

**Files Affected:** `protocol/v1/impl/MarginBank.sol`, `protocol/v1/PerpetualV1.sol`, `protocol/v1/impl/Debt.sol`

**Severity: Medium**

There is an inconsitency between the way that admin functions are implemented. For example, `Admin.sol` has `setGlobalOperator()` defined which controls [setting the global operator](https://github.com/dtradeorg/orderbook-contracts/blob/qs-audits/contracts/protocol/v1/impl/Admin.sol#L52). This is also present in [MarginBank.sol](https://github.com/dtradeorg/orderbook-contracts/blob/qs-audits/contracts/protocol/v1/impl/MarginBank.sol#L258) though the implementations differ. The check for the valid MarginBank ERC20 could be bypassed by calling the other `setGlobalOperator()` from `Admin.sol` and adding the `_GLOBAL_OPERATORS_[operator]` though a different call.

**Recommendations:**

Move to using a single admin implementation that's used for all admin operations and modifiers across the contracts.

**[12] Unprotected Admin Function**

**Files Affected:** `protocol/v1/impl/Operator.sol`

**Severity: High**

[`setLocalOperator()`](https://github.com/dtradeorg/orderbook-contracts/blob/qs-audits/contracts/protocol/v1/impl/Operator.sol#L36) may be called by anybody, allowing a state change of the calling party to added to `_LOCAL_OPERATORS_`

**Recommendations:**

Clarify whether this is intentional or add a modifier to prevent this function from being called from a third party.

**[13] Large Number of accounts can lead to DoS**

**Files Affected:** `protocol/v1/impl/Settlement.sol`

**Severity: Medium**

Within `_settleAccounts` there is a [`loop`](https://github.com/dtradeorg/orderbook-contracts/blob/qs-audits/contracts/protocol/v1/impl/Settlement.sol#L144) which will iterate through all accounts. If the size of this accounts array contains a large number of users, this function will never complete.

**Recommendations:**

Consider adding an upper bound of number of accounts to settle to always ensure this function will execute correctly.

**[14] Traders can trade with themselves**

**Files Affected:** `protocol/v1/impl/Trade.sol`

**Severity: Medium**

Within `trade()` there is a check that the maker and taker are the [same account](https://github.com/dtradeorg/orderbook-contracts/blob/qs-audits/contracts/protocol/v1/impl/Trade.sol#L125). The maker and taker should be different accounts and they should not be allowed to trade with each other.

**Recommendations:**

Do not allow users to trade with themselves.

**[15] Incomplete Implementation**

**Files Affected:** `protocol/v1/traders/Trader.sol`

**Severity: Medium**

This contract is incomplete, yet other contracts import Trader.sol and could be relying on its functionality.

**Recommendations:**

Fully implement this contract or remove the file and imports to it.

## Best Practices

**[1] Update docstring**

**Files Affected:** `contracts/TradeMining.sol`

The docstring on [`line 14`](https://github.com/dtradeorg/dtrade_v1/blob/main/contracts/TradeMining.sol#L14) should be updated to reflect the migrated token ticker. DET -> DTX on Kusama, DET -> FFLY in production.

Same applies to variable assignment and function argument name `detToken`.

**[2] Variable Missing Docstring**

**Files Affected:** `contracts/TradeMining.sol`

Variable [`totalRewardsToDistribute`](https://github.com/dtradeorg/dtrade_v1/blob/main/contracts/TradeMining.sol#L95) is missing a docstring.

**[3] Typo in Docstring**

**Files Affected:** `contracts/TradeMining.sol`

[`rewrds`](https://github.com/dtradeorg/dtrade_v1/blob/main/contracts/TradeMining.sol#L97) -> rewards

**[4] Unnecessary Comment**

**Files Affected:** `contracts/TradeInfo.sol`

No enums are [defined](https://github.com/dtradeorg/dtrade_v1/blob/main/contracts/TradeInfo.sol#L20)

**[5] Leading blank lines can be removed**

**Files Affected:** `protocol/v1/impl/FinalSettlement.sol`, `protocol/v1/lib/BalanceMath.sol`, `protocol/v1/impl/Operator.sol`, `protocol/v1/intf/I_Funder.sol`,
`protocol/v1/intf/I_Oracle.sol`,
`protocol/v1/intf/I_Trader.sol`,
`protocol/v1/lib/BalanceMath.sol`,
`protocol/v1/lib/IndexMath.sol`,
`contracts/protocol/v1/lib/Types.sol`,
`protocol/v1/oracles/FundingOracle.sol`,
`protocol/v1/oracles/InverseFundingOracle.sol`,
`protocol/v1/traders/type/OrderTrader.sol`,
`/protocol/v1/traders/Trader.sol`,
`protocol/v1/traders/TraderConstants.sol`,
`protocol/PerpetualProxy.sol`