Audit Report
dHedge Platform Smart Contract Audit

# 1. Introduction
iosiro was commissioned by [dHedge]( to conduct an audit of the dHedge Platform smart contracts. The audit was performed between 2 and 14 September 2020. A review of changes was performed on 2 and 9 October 2020.

This report is organized into the following sections.

* **[Section 2 - Executive Summary:](#section-2)** A high-level description of the findings of the audit.
* **[Section 3 - Audit Details:](#section-3)** A description of the scope and methodology of the audit.
* **[Section 4 - Design Specification:](#section-4)** An outline of the intended functionality of the smart contracts.
* **[Section 5 - Detailed Findings:](#section-5)** Detailed descriptions of the findings of the audit.

The information in this report should be used to better understand the risk exposure of the smart contracts, and as a guide to improving the security posture of the smart contracts by remediating issues identified. The results of this audit are only a reflection of the source code reviewed at the time of the audit and of the source code that was determined to be in-scope.

The purpose of this audit was to achieve the following:

* Identify potential security flaws.
* Ensure that the smart contracts functioned according to the documentation provided.

Assessing the economics, game theory, or underlying business model of the platform were strictly beyond the scope of this audit.

Due to the unregulated nature and ease of transfer of cryptocurrencies, operations that store or interact with these assets are considered very high risk with regards to cyber attacks. As such, the highest level of security should be observed when interacting with these assets. This requires a forward-thinking approach, which takes into account the new and experimental nature of blockchain technologies. Strategies that should be used to encourage secure code development include:

* Security should be integrated into the development lifecycle and the level of perceived security should not be limited to a single code audit.
* Defensive programming should be employed to account for unforeseen circumstances.
* Current best practices should be followed where possible.

<a name="section-2"></a>
# 2. Executive Summary

The purpose of the dHedge platform was to create a platform for non-custodial mimetic trading of synthetic assets on Ethereum using the [Synthetix]( platform. The system allowed users to create and manage funds. Other users would be able to deposit into these funds, allowing the fund manager to trade on their behalf. Upon exiting the fund, users would receive a percentage of the fund assets commensurate with their total deposit. For a full specification see [Section 4 - Design Specification](#section-4).

Several issues were identified during the audit, including two high risk vulnerabilities relating to the system's integration with Synthetix and the use of functionality implemented in [SIP-37]( At the conclusion of the initial audit, the high risk issues had been addressed while several informational issues remained open. After further changes had been made to the codebase, a review was conducted to confirm that they had been adequately addressed. At the conclusion of the review, two informational issues remained open.  The code accorded with its specification and was of a high standard.

<a name="section-3"></a>
# 3. Audit Details

## 3.1 Scope

The source code considered in-scope for the assessment is described below. Code from all other files is considered to be out-of-scope. Out-of-scope code that interacts with in-scope code is assumed to function as intended and introduce no functional or security vulnerabilities for the purposes of this audit.

### 3.1.1 dHedge Smart Contracts

**Project Name:** dHedge System<br/>
**First Audit Commit:** [6d56944](<br/>
**Last Audit Commit:** [62a3e6b](<br/>
**Files:** DHedge.sol, DHedgeFactory.sol, Managed.sol

### 3.1.2 dHedge Smart Contracts Review

**Project Name:** dHedge System<br/>
**First Review Commit:** [c84e114](<br/>
**Final Review Commit:** [5fbbc4b](<br/>
**Files:** DHedge.sol, DHedgeFactory.sol, Managed.sol

## 3.2  Methodology

A variety of techniques were used while conducting the audit. These techniques are briefly described below.

### 3.2.1 Code Review

The source code was manually inspected to identify potential security flaws. Code review is a useful approach for detecting security flaws, discrepancies between the specification and implementation, design improvements, and high risk areas of the system.

### 3.2.2 Dynamic Analysis

The contracts were compiled, deployed, and tested in a Ganache test environment. Manual analysis was used to confirm that the code operated at a functional level, and to verify the exploitability of any potential security issues identified. The coverage report of the provided Truffle tests as on the final day of the audit is given below.

File                                   |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
contracts/                            |    86.29 |    75.51 |    83.75 |    86.42 |                |
 DHedge.sol                           |    85.22 |    82.35 |    84.21 |    85.53 | 172,466,507,511,538, 589,729 |
 DHedgeFactory.sol                    |    91.53 |       50 |    82.14 |    90.16 | 125,135,160,191,256 |
 IAddressResolver.sol                 |      100 |      100 |      100 |      100 |                |
 IExchangeRates.sol                   |      100 |      100 |      100 |      100 |                |
 IExchanger.sol                       |      100 |      100 |      100 |      100 |                |
 IHasAssetInfo.sol                    |      100 |      100 |      100 |      100 |                |
 IHasDaoInfo.sol                      |      100 |      100 |      100 |      100 |                |
 IHasFeeInfo.sol                      |      100 |      100 |      100 |      100 |                |
 ISynth.sol                           |      100 |      100 |      100 |      100 |                |
 ISynthetix.sol                       |      100 |      100 |      100 |      100 |                |
 ISystemStatus.sol                    |      100 |      100 |      100 |      100 |                |
 Managed.sol                          |    84.38 |       80 |    85.71 |    85.71 | 44,51,52,53,76 |
contracts/mocks/                      |    93.75 |    66.67 |    89.66 |    93.85 |                |
 IbtcToken.sol                        |      100 |      100 |      100 |      100 |                |
 IethToken.sol                        |      100 |      100 |      100 |      100 |                |
 MockAddressResolver.sol              |    85.71 |       50 |      100 |     87.5 |             17 |
 MockExchangeRates.sol                |    92.31 |      100 |    66.67 |    92.31 |             62 |
 MockExchanger.sol                    |       50 |      100 |       50 |       50 |             20 |
 MockSynth.sol                        |    93.33 |       50 |    88.89 |    93.33 |             76 |
 MockSynthetix.sol                    |      100 |      100 |      100 |      100 |                |
 MockSystemStatus.sol                 |      100 |      100 |      100 |      100 |                |
 SbtcToken.sol                        |      100 |      100 |      100 |      100 |                |
 SethToken.sol                        |      100 |      100 |      100 |      100 |                |
 SlinkToken.sol                       |      100 |      100 |      100 |      100 |                |
 SusdToken.sol                        |      100 |      100 |      100 |      100 |                |
contracts/test/                       |    96.67 |       50 |       90 |    96.67 |                |
 DHedgeFactoryV2.sol                  |      100 |      100 |      100 |      100 |                |
 DHedgeFactoryV3.sol                  |      100 |      100 |      100 |      100 |                |
 DHedgeV2.sol                         |      100 |      100 |      100 |      100 |                |
 DHedgeV3WithExitFees.sol             |      100 |       50 |      100 |      100 |                |
 TestImplementationV1.sol             |      100 |      100 |      100 |      100 |                |
 TestImplementationV2.sol             |    66.67 |      100 |    66.67 |    66.67 |              7 |
contracts/upgradability/              |    93.75 |    58.33 |    93.33 |    91.89 |                |
 Address.sol                          |      100 |      100 |      100 |      100 |                |
 BaseUpgradeabilityProxy.sol          |    77.78 |       75 |    66.67 |       80 |          50,51 |
 HasLogic.sol                         |      100 |      100 |      100 |      100 |                |
 InitializableUpgradeabilityProxy.sol |      100 |       50 |      100 |      100 |                |
 Proxy.sol                            |      100 |      100 |      100 |       80 |             20 |
 ProxyFactory.sol                     |      100 |      100 |      100 |      100 |                |
All files                              |    88.59 |    72.13 |    86.57 |     88.6 |                |

Gaps in test coverage are discussed in [5.4.1 Insufficient Unit Test Quality and Coverage](#test-coverage) below.

### 3.2.3 Automated Analysis

Tools were used to automatically detect the presence of several types of security vulnerabilities, including reentrancy, timestamp dependency bugs, and transaction-ordering dependency bugs. The static analysis results were manually analysed to remove false positive results. True positive results would be indicated in this report. Static analysis tools commonly used include Slither, MythX, as well as Securify. Furthermore, the Remix IDE, compilation output, and linters are also used to identify potential areas of concern.

## 3.3  Risk Ratings

Each issue identified during the audit has been assigned a risk rating. The rating is determined based on the criteria outlined below.

* **High Risk** - The issue could result in a loss of funds for the contract owner or system users.
* **Medium Risk** - The issue resulted in the code specification being implemented incorrectly.
* **Low Risk** - A best practice or design issue that could affect the security of the contract.
* **Informational** - A lapse in best practice or a suboptimal design pattern that has a minimal risk of affecting the security of the contract.
* **Closed** - The issue was identified during the audit and has since been addressed to a satisfactory level to remove the risk that it posed.

<a name="section-4"></a>
# 4. Design Specification
The following section outlines the intended functionality of the system at a high level.

## 4.1 dHedge Smart Contracts

The specification given below was derived by iosiro from the code in the target commit hashes. Any discrepancies between this specification and expected behaviour should be brought to the attention of iosiro.

### 4.1.1 Overview

The dHedge system allowed anyone to mimic the trades of fund managers on the [Synthetix]( platform. Users would enter funds with Synthetix USD (sUSD) and exit with a proportion of the underlying assets. Managers are allowed to exchange the underlying fund assets to any other supported assets, but not to withdraw other users funds'.

Managers are awarded two kinds of fees:

* A manager fee based on the performance of the underlying assets, denominated in fund tokens. The amount of this fee is determined by the increase in value (in sUSD) of the fund's total assets. Available manager fees can be withdrawn at any time by the manager. On withdrawal, a portion of this fee is sent to the dHedge DAO.
* An exit fee, denominated in fund tokens. This fee is sent to the manager when users withdraw from the fund if the funds are withdrawn before a DAO-set waiting period.

Managers set both fees as percentages when creating a fund. They can decrease, but not increase these percentages once the fund has been created.

### 4.1.2 dHedge System Specification

#### Funds

dHedge funds had the following specification:

✅ Anyone is able to create a new fund and become a manager.<br/>
✅ Funds can be public and available to everyone, or private and available for whitelisted users. The fund manager can toggle existing funds between these states at will.<br/>
✅ Investors can join a fund with sUSD and receive a fund token.<br/>
✅ Fund tokens can be redeemed for a proportion of the fund's underlying assets.<br/>
✅ If a fund becomes private, non-whitelisted investors cannot deposit more capital, but can withdraw from the fund using their existing fund tokens.

#### Fund Managers

dHedge managers had the following specification:

✅ Managers can invest in their own and other pools.<br/>
✅ Managers can set their own performance and exit fee rates. The fee rates can be changed lower, but not higher.<br/>
✅ A portion of the manager's performance fee is directed to a dHedge DAO address.<br/>
✅ Managers cannot withdraw underlying fund assets in excess of their own contributions.<br/>
✅ Managers can enable and disable Synthetix assets, except persistent assets such as sUSD.<br/>
✅ Managers cannot disable assets with a non-zero balance.  

#### Upgradeability

The dHedge System had the following upgradeability specification:

✅ The platform is based on OpenZeppelin upgradeable contracts.<br/>
✅ The logic contract of funds can be updated without moving the underlying assets.

<a name="section-5"></a>
# 5. Detailed Findings
The following section includes in depth descriptions of the findings of the audit.

## 5.1 High Risk

No high risk issues were present at the conclusion of the audit.

## 5.2 Medium Risk

No medium risk issues were present at the conclusion of the audit.

## 5.3 Low Risk

No low risk issues were present at the conclusion of the audit.

## 5.4 Informational

<a name="test-coverage"></a>

### 5.4.1 Insufficient Unit Test Quality and Coverage

#### Description

Certain functionality was found to have insufficient unit test quality and coverage. To improve the maintainability of the code, and decrease the likelihood of introducing functional or security issues into the codebase, it is recommended that the test coverage be extended.

#### Remedial Action

The test suite should be extended to cover the following:

* In `DHedge.sol`: The `getFundSummary()`, `getSupportedAssets()`, `getFundComposition()`, `getWaitingPeriods()`, `tokenPrice()`, and `getExitFeeCooldown()` functions.
* In `DHedgeFactory.sol`: The `setDaoAddress()`, `setDaoFee()`, `onlyPool()`, `getMaximumManagerFee()`  and `setTrackingCode()` functions.
* In `Managed.sol`: The `getMembers()` and `changeManager(...)` function.

### 5.4.2 Design Comments

Actions to improve the functionality and readability of the codebase are outlined below.

<a name="inefficient-settlement-functionality"></a>

#### Inefficient Settlement Functionality

The `settleAll()` function is called each time a user deposits or withdraws funds, and when the manager mints their fee. This function can be relatively expensive, as each supported asset needs to be settled before normal functionality can resume. A more efficient approach would be to set a variable called `timeOfLastExchange` when an exchange happens. Once `SystemSettings.waitingPeriodSecs()` has passed since `timeOfLastExchange`, then a user can run `settleAll()`. This removes the need for users to waste gas on calling `settle()` to reach the revert case, and it also prevents users from attempting to settle before the waiting period has passed.

##  5.5 Closed

<a name="section-5.5.1"></a>

### 5.5.1 Synthetix Settlements Not Accounted for on Deposit or Withdraw (High Risk)
*DHedge.deposit(...), DHedge.withdraw(...)*

#### Description

In [SIP-37](, Synthetix implemented functionality to prevent front-running on synth exchanges in the form of reclamations and rebates, handled through the `Synthetix.settle(...)` and `Exchanger.settle(...)` functions. In practice, this means that a user or contract's synth balances are liable to change following the invocation of either of these functions, which is necessary to transfer, exchange and burn synths.

The dHedge contract invoked settlement on deposits and withdrawals through the use of `transferAndSettle(...)`, but did not take the adjusted balances into account for calculations involving the amount deposited or withdrawn, using the unsettled balance instead. This could allow depositors to receive more DHF tokens than their sUSD deposit is worth, essentially exploiting a dHedge fund to maintain profit from front-running a Synthetix exchange at the expense of the other fund members.

This vulnerability could also cause withdrawal by the final member of a dHedge fund to fail, or tokens to be locked in the contract, should a settlement change the contract's token balance during a withdrawal.

#### Remedial Action

To ensure accurate accounting, settlements should be accounted for prior to the calculations required for depositing and withdrawing funds. For deposits, the depositor's synth balance should be settled, and for withdrawals, the fund's synth balances should be settled. In both cases, the fund's synth balance should be settled before calculating the total fund value.

#### Update

dHedge added the following commits to address this vulnerability:

* [ef05540]( Settle the synth balance before transfer when withdrawing.
* [01e08f8]( Settle the synth balance before transfer while depositing.
* [e518349]( Implemented `settleAll()` function to settle all synth balances before determining the total fund value when withdrawing, depositing or minting the manager fee.
* [62a3e6b]( Settle the synth balance of the depositor instead of the fund while depositing.

During the audit, an informational issue was found with the changes made in `e518349`, which is detailed in [Inefficient Settlement Functionality](#inefficient-settlement-functionality).

### 5.5.2 Synthetix Settlements Not Accounted for on Exchange (High Risk)

#### Description

Similarly to [5.5.1](#section-5.5.1) above, attackers could use a dHedge fund to profit from Synthetix rebates. As an example, consider a fund that has just made an exchange. The fund's total value is $1 million, and due to an unfavorable price change, it is owed a rebate of $10 000. An attacker could use a flash loan to deposit $9 million sUSD into the fund, increasing the total fund value to $10 million and claiming 90% of the fund's assets. In the same transaction, the attacker could call `Exchanger.settle(...)` on the fund contract and the synth to be rebated. This would increase the fund's value to $10.01 million, after which the attacker could withdraw $9.009 million minus exit fees, for a profit of approximately $9000. This attack could also be carried out in multiple transactions, without a flash loan.

#### Remedial Action

To prevent rebate attacks, iosiro recommends disabling deposits and withdrawals after an exchange until the system has been settled.

#### Update

dHedge added the following commit, which addresses this vulnerability:

* [e518349]( Implemented `settleAll()` function to settle all synth balances before determining the total fund value when withdrawing, depositing or minting the manager fee.

However, an informational issue was found with the changes made in this commit, which is detailed in  [Inefficient Settlement Functionality](#inefficient-settlement-functionality).

### 5.5.3 Potentially Unsafe Withdraw Functionality (High Risk)

#### Description

During the audit review, it was found that the withdraw functionality had been adapted to forfeit any funds held by a user if the asset was frozen by the Synthetix system. This allowed a user to retrieve their stake from the system even in the event that one of the synths was frozen. However, as synths could be frozen for several reasons (e.g. when traditional markets are closed, during a system upgrade, in the event of an issue with the price oracle triggering the [circuit breaker](, etc.) users could lose a significant portion of their funds if they inadvertently withdrew their funds while a synth was frozen.

#### Remedial Action

It is recommended that the original withdraw functionality be restored in order to prevent users from accidentally withdrawing funds from the system while a synth is frozen. A separate function can be implemented to provide users with the ability to exit from the system if they intend to do so.

#### Update

A separate withdraw function was implement in [5fbbc4b](

### 5.5.4 No Constraint on DAO Fee (Informational)

#### Description

The `DHedgeFactory` contract allowed its owner to change the DAO fee at any time and did not set any constraints on this fee. Thus the contract owner could:

* Set the DAO fee to 100%, which would result in the entire manager fee being taken by the DAO.
* Set the DAO fee above 100%, which would cause calls to the `DHedge.mintManagerFee()` to revert with a SafeMath subtraction overflow.

#### Remedial Action

To enhance user trust in the dHedge system and prevent accidental contract breakage, iosiro recommends setting and validating a maximum DAO fee amount.

#### Update

The DAO fee can no longer be set above 100%, as implemented in [f91b2dd](

### 5.5.5 Missing Validation for Manager and Exit Fee Rates (Informational)

#### Description

The specification included that managers were allowed to change their performance and exit fee rates lower, but not higher, than the current set rate. However, both fee rates could be set higher than the current rate due to missing validation in the `initialize(...)` function. This function can only be called during contract upgrades, which lowered the risk of this vulnerability.

#### Remedial Action

iosiro recommends storing the performance and exit fee rates in the `DHedgeFactory` when creating a new fund. The rates can be stored as a mapping associated to a fund, and can then be used for validation in the fund `initialize(...)` function.

Alternatively, an upper bound for fee rates can be determined and stored as a constant in each fund. Validation can then be added to ensure that any fee rate set is below or equal to the upper bound rate.

#### Update

The manager fee was capped to a maximum of 50% in [f21ff5c](

### 5.5.6 Potentially Unsafe Arithmetic Used (Informational)

*[DHedge.sol#L236](, [DHedge.sol#L238](, [DHedge.sol#L242](*, [DHedge.sol#L336](*, [DHedge.sol#L337](*

#### Description

The `_removeFromSupportedAssets` function made use of the unsafe `+` and `-` operators instead of functions that prevented overflows, such as the SafeMath `add` and `sub` functions. However, additional validation from the `removeFromSupportedAssets` function lowered the risk of the possibility of an overflow.

The `Deposit` event in the `deposit(...)` function also made use of the `+` operator; however, it was unlikely that an overflow or underflow would occur. Moreover, the risk of an overflow in an event emission is lowered as events are only stored in transaction logs and not used by the contract.

#### Remedial Action

To mitigate the risk of overflows, use `SafeMath.add()` in place of `+` and `SafeMath.sub()` in place of `-`.

#### Update

Implemented in [2ec55d2](

### 5.5.7 Closed Design Comments (Informational)

#### Inconsistent usage of `pool` and `msg.sender`

`DHedgeFactory.setPoolManagerFeeNumerator` used both a function parameter `pool` address and `msg.sender`, without validating that the `pool` was in fact the `msg.sender`. While in the current implementation no potential risk was identified, validation should be included to ensure that a pool is only able to change its own manager fee.

#### Update

Additional validation was implemented in [58e0bd0]( to validate that the `msg.sender` was the pool.

#### Refactoring Suggestions

It is recommended that certain portions of the code be refactored to improve readability and consistency, as indicated below.

* ``: Settling the destination synth before an exchange is not necessary.
* `DHedge.persistentAsset[_SUSD_KEY]`, `DHedge.deleteFund(...)`: These components have outstanding TODO items.
* [In Solidity version 0.7.0, the `now` keyword was deprecated.]( Developers are encouraged to use `block.timestamp` instead, to ensure forward compatibility.
* `Managed.sol`: A `__gap` state variable should be included in this contract to allow future upgrades to add additional state variables.

##### Update

* Unnecessary settlement removed in [9b1b5b8](
* Removed `deleteFund()` functionality and associated TODO comment in [20372b3](
* Removed TODO comment for `persistentAssets[...]` in [2d7d458]( This functionality will remain in DHedge in the short term.
* The `now` keyword was replaced with `block.timestamp` in [f5cb7fd](
* A `__gap` was added into `Managed.sol` in [e97470d](

#### Removing Support for an Asset Should Require Settlement

When removing support for an asset, it was required to have a balance of zero. However, it was still possible to remove a zero-balance asset that had not been settled. While it would be possible to simply add the asset back to retrieve the outstanding balance, it is recommended that assets are settled before being removed to ensure that there is no outstanding balance.

##### Update

Implemented in [4ce2243](

#### Fix Spelling and Naming Convention Errors

Spelling mistakes and contraventions of Solidity naming conventions were identified in the codebase. Fixing these mistakes can help improve the end-user experience by providing clear information on errors encountered, and improve the maintainability and auditability of the codebase.

* `DHedge.sol#L302,L363,L526`: "asstes" should be "assets"
* `DHedge.sol#L342`: The internal function name `settleAll` should be `_settleAll`.
* `HasDaoInfo.sol`: As this contract is an interface, it should be named `IHasDaoInfo`.

##### Update

* Fixed in [feaa394](
* Fixed in [21820d6](
* Fixed in [f4a14ee](

Secure your system.
Request a service
Start Now