# Dyad v6 Security Review

**Disclaimer:** This is a report detailing the findings uncovered during the solo security review of the Dyad smart contract codebase. While best efforts were made to identify all relevant issues affecting asset security, general correctness and user experience it is by no means a guarantee that the codebase is free from bugs or other flaws.

## Index

[0 Index](#Index)
[1 Overview](#Overview)
[1.1 Scope](#Scope)
[1.2 Methodology](#Methodology)
[2 Findings](#Findings)
[2.1 Findings Classification](#Findings-Classification)
[2.2 Access Control Issues](#Access-Control-Issues)
[2.2.1 [H-1] Licenser Owners Can Mint Arbitrary Amounts of Unbacked DYAD](#AccessH-1-Licenser-Owners-Can-Mint-Arbitrary-Amounts-of-Unbacked-DYAD)
[2.2.2 [H-2] Vault Licenser Can Steal Borrower Collateral](#AccessH-2-Vault-Licenser-Can-Steal-Borrower-Collateral)
[2.3 Core issues](#Core-Issues)
[2.3.1 [C-1] Liquidators Can Be Frontrun To Make Liquidation Unprofitable](#CoreC-01-Liquidators-Can-Be-Frontrun-To-Make-Liquidation-Unprofitable)
[2.3.2 [H-1] Inefficient Liquidations](#CoreH-1-Inefficient-Liquidations)
[2.3.3 [M-1] Incorrect Asset Price Denomination](#CoreM-1-Incorrect-Asset-Price-Denomination)
[2.3.4 [M-2] Incorrect Checking Of Stale Oracle Data](#CoreM-2-Incorrect-Checking-Of-Stale-Oracle-Data)
[2.3.5 [M-3] Incorrect Oracle Address Configured For Deployment](#CoreM-3-Incorrect-Oracle-Address-Configured-For-Deployment)
[2.3.6 [M-4] Stale Oracle Data Allows the Sytem to Accrue Bad Debt](#CoreM-4-Stale-Oracle-Data-Allows-the-Sytem-to-Accrue-Bad-Debt)
[2.3.7 [L-1] Incorrect Decimal Handling in `VaultManager.redeemDyad`](#CoreL-1-Incorrect-Decimal-Handling-in-VaultManagerredeemDyad)
[2.4 Informational Issues](#Informational-Issues)
[3 Appendix](#Appendix)
[3.1 Gas Optimization](#Gas-Optimization)

## Overview

**Participating Security Researcher & Report Author:** Philogy ([@real_philogy](https://twitter.com/real_philogy))
**Duration of Engagement:** 4 days (28.11.2023 - 01.12.2023)

**Access-Control Findings Overview**

|Impact|Total Found|Fixed|Unresolved |
|--------|-----------|------------|--------|
|High|2|0|2x Acknowledged|
|Medium|0|0|0|
|Low|0|0|0|

**Core Findings Overview**

|Severity|Total Found|Fixed|Unresolved|
|--------|-----------|--------|------|
|Critical|1|1|0|
|High|1|1*|0|
|Medium|4|3|1 x Acknowledged |
|Low|1|1|0|

\* The suggested simpler, short-term mitigation was implemented. Long-term a more efficient solution should be implemented for that issue.

### Scope
Repository: https://github.com/DyadStablecoin/contracts-v6
Initial commit: [`a853984`](https://github.com/DyadStablecoin/contracts-v6/tree/a8539841ab4a2960dfee523143d9c62d421d40b4)
Final commit: [`d6049b3`](https://github.com/DyadStablecoin/contracts-v6/tree/d6049b3fb45529d51f4152907a62b1e21bac8c79)
Files:
- All files under `src/`, excluding:
- `src/params/*`
- `interfaces/IDNft.sol`
- `src/core/DNft.sol`
- Constants in `src/Parameters.sol` prefixed with `GOERLI_`
- `script/deploy/DeployBase.s.sol`
- `script/deploy/Deploy.Mainnet.s.sol`

### Methodology

The issues were mainly identified via manual review of the code in scope. Stateful fuzzing via so-called "invariant testing" was also applied.

## Findings

### Findings Classification

The severity of findings is determined by their determined likelihood and impact as dictated by the below matrix:

|Likelihood / Impact|High|Medium|Low|
|-----|-----------|------------|--------|
|**High** |Critical|High|Medium|
|**Medium**|High|Medium|Low|
|**Low** |Medium|Low|Low|

Regardless of severity it is recommended that all issues are addressed, even issues with a mere Low severity _can be_ signs of sub-optimal processes and can manifest to be more severe issues with later modifications to the overall code & system.

### Access-Control Issues

This section describes capabilities or features of the system that may/may not be intentional but grant specific actors significant privileges that may harm other users if abused. Unlike core issues access control issues are merely classified by their impact if abused.

#### [Access/H-1] Licenser Owners Can Mint Arbitrary Amounts of Unbacked DYAD

**Context:**
- [`src/core/Dyad.sol#L26-L35`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/core/Dyad.sol#L26-L35)
- [`src/core/VaultManager#L184-L201`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/core/VaultManager.sol#L184-L201)

**Description:**

The Dyad system generally has 2 separate licensers (that may be controlled by the same party) that track a list of _licensed_ vaults and _DYAD_ issuers. Modifying these lists is permissioned and limited to the `owner()` address of the respective licenser.

Licensed vaults have the ability to store and report value to the vault manager. Abused the owner can create and license malicious vaults that report entirely fictional collateral values, allowing the owner to mint arbitrary Dyad via the manager (assuming the own a DNft, which is purchasable by anyone).

Similarly licensed dyad minters are able to mint DYAD directly. If the owner of the minter licenser abuses their permissions they could mint arbitrary amounts of Dyad.

In both situations this would allow the licenser owners to acquire and sell massive amounts of unbacked Dyad, leading to a loss of funds of Dyad holders and liquidity providers.

**Recommendation:**

Ensure that the control over the licenser is at the very least secured by a multi-sig with doxxed, accountable members. Furthermore a time-lock with an execution delay of at least 1 week should be placed between the owner's input and the licenser such that an intended malicious change could be observed in advance and possibly give time to participants in the system to willfully exit.

**Status:**

Acknowledged, determined to be fixed in the future.

#### [Access/H-2] Vault Licenser Can Steal Borrower Collateral

**Context:**
- [`src/core/VaultManager.sol#L195-197`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/core/VaultManager.sol#L195-L197)
- [`src/core/Licenser.sol#L13`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/core/Licenser.sol#L13)

**Description:**

The vault licenser not only has the ability to _license_ new vaults, enabling them as collateral storers in the vault manager but also to "de-license" them, removing them as valid collateral providers.

Doing this for an existing, still in use vault would have the effect that all Dyad borrowers would have their collateral value instantly drop by the vaule of collateral they supplied via that vault. This would allow the owner to instantly liquidate borrowers who relied on that collateral type to keep their positions healthy. The licenser owner could liquidate everyone by simply removing all vaults.

**Recommendation:**

It is recommended that a safer vault off-boarding mechanism is created, such as only allowing the owner to pause additional borrows with a certain collateral type but disallowing the complete, instant removal.

Furthermore as recommended in `Access/H-1` add a time-lock and enforce multi-party approval for any actions that could be abused to harm users.

**Status:**

Acknowledged, determined to be fixed in the future.

### Core Issues

This section describes the core logic issues in the provided contracts. These are ways in which the system may function in unintentional way or generally be sub-optimal.

Upon the clients' request gas findings were not looked for. High-level suggestions that happened to be discovered anyway are detailed in the appendix.

#### [Core/C-01] Liquidators Can Be Frontrun To Make Liquidation Unprofitable

**Context:** [`src/core/VaultManager.sol#L65-L85`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/core/VaultManager.sol#L65-L85)
**Description:**
Borrowers in the Dyad system can remove and add vaults they wish to depend on as collateral for their positions. However borrowers can remove vaults they are currently using and relying upon for collateral. While this reduces their collateral value, likely making their position subject to liquidation it also removes the ability for liquidators to claim those assets.

This is problematic for several reasons. Firstly this can lead to the system accruing bad debt as borrowers deposit collateral, create debt and remove the incentive for that debt to be naturally repaid by a 3rd party.

Secondly borrowers could exploit this to get liquidators to repay their debt for free. The exploit scenario would be:
1. A user deposits collateral and borrows DYAD.
2. As the price of their collateral drops their collateralization ratio (CR) drops below the minimum 150%, making them subject to liquidation.
3. Seeing this a liquidatior submits a transaction to liquidate the user.
4. The borrower who's about to be liquidated frontruns the liquidation transactions by removing the underwater collateral as one of their enabled vaults via the `VaultManager.remove` function
5. The liquidators transaction gets processed, as the CR of the borrower is even lower now it is still accepted, burning the liquidator's DYAD to repay the users debt, but since the vault is removed from the list no collateral is given to the liquidator
6. The borrower now restores the vault via `VaultManager.add`, with no debt they are able to withdraw their original collateral

**Recommendation:**

Ensure that borrowers are not able to remove vaults while being in debt & having collateral in them.

**Status:**

Confirmed and fixed by shafu0x in [`fcfee9e`](https://github.com/DyadStablecoin/contracts-v6/commit/fcfee9eb8995f0ff064d514163d48b4fc8027dfe)

#### [Core/H-1] Inefficient Liquidations

**Context:** [`src/core/VaultManager.sol#L154-L171`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/core/VaultManager.sol#L154-L171)

**Description:**

When the collateralization ratio (CR) of a user's position drops below 150% the `VaultManager` allows 3rd parties (other DNft owners) to liquidate the position. This is done to ensure the system does not accrue any bad debt.

To incentivize the liquidation the liquidator who repays the debt of the position receives 100% of the remaining collateral. This is a relatively large price to pay, especially when you consider that the only value the liquidator provides is:
- swap execution between DYAD <> underlying collateral
- gas fee for liquidation execution
- cost of infrastructure / attention to notice and take advantage of liquidation opportunities

For a larger position the liquidation reward may be multiple thousand $ worth of crypto assets. Liquidations are usually highly competitive with most of the value eventually being bid up and fed to proposers. This is inefficient as with a better liquidation mechanism the capturing of that value could be enshrined, leaving it with the user or another part of the protocol.

**Recommendation:**
As an initial, simpler mitigation it is recommended to only reward the liquidator with a portion of the borrower's equity (collateral value - debt). This can be implemented by:
1. Allowing you to specify an `amount` to be transferred in the `Vault.move{...}` function
2. Doing the partial equity calculation in `liquidate`:

function liquidate(uint256 id, uint256 to) external isValidDNft(id) isValidDNft(to) {
    uint256 cr = collatRatio(id);
    if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
    dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

    uint256 cappedCr = cr < 1e18 ? 1e18 : cr;
    uint256 liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
    uint256 liquidationAssetShare = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

    uint256 numberOfVaults = vaults[id].length;
    for (uint256 i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[id][i]);
        uint256 collateral = vault.id2asset(id).mulWadDown(liquidationAssetShare);
        vault.move(id, to, collateral);
    }
    emit Liquidate(id, msg.sender, to);
}

The new calculation essentially calculates the share of assets to be given to the liquidator to cover the debt + some percentage characterized by the `LIQUIDATION_REWARD` as a share of the position's equity.

A better solution long-term would be to have the protocol emit an intent "sell the collateral $x_0, x_1, ..., x_n$ in exchange for $y$ Dyad tokens" routed to a system that is equipped to competitively price and execute the intent to minimize the protocol's cost of liquidation.

**Status:**

Simpler initial mitigation fixed and implemented by shaf0x as of [`5a35286`](https://github.com/DyadStablecoin/contracts-v6/commit/5a352860622364c4dc9014fb64a3dee4f6c79a27).

#### [Core/M-1] Incorrect Asset Price Denomination

**Context:** [`src/core/Vault.sol#L83`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/core/Vault.sol#L83)

**Description:**

When reporting the USD value of a position via the `getUsdValue` method the `Vault` contract directly queries its underyling oracle and then proceeds to cancels out the oracle denomination (`assetPrice` queries the oracle):

id2asset[id] * assetPrice() / 10**oracle.decimals()

The issue is that `id2asset` tracks the asset amounts of user's collateral deposits. These are denominated in the decimals of the underlying asset, for assets that share the same denomination as DYAD ($10^{18}$) this presents no problems, however for tokens that have more or less than 18 decimals this will lead to the vault over/under-reporting the value of assets.

The few popular tokens that deviate from 18 decimals typically have *fewer* decimals e.g. USDC has 6. Which would only lead to the under-reporting of value, allowing users to borrow less. Nevertheless if a token with more than 18 decimals its value would be **overstated** by a factor 10 for every decimal difference, allowing users to mint undercollateralized DYAD.

**Recommendation:**

Ensure that valuing functions take the denomination of underlying tokens into recommendation, `getUsdValue` should be changed to multiply by a further `1e18` and divide by `10**asset.decimals()` to ensure the final value is denominated in $10^{18}$.

**Status:**

Confirmed and fixed by shafu0x in [`a46e716`](https://github.com/DyadStablecoin/contracts-v6/commit/a46e716d3f5f54f46c6335bffbbe4d01d705548f).

#### [Core/M-2] Incorrect Checking Of Stale Oracle Data
**Context:** [`src/core/Vault#L90-L99`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/core/Vault.sol#L90-L99)

**Description:**

When querying the underlying chainlink oracle the `Vault` contract does not have a useful oracle data staleness check:

function assetPrice() public view returns (uint256) {
    (uint80 roundID, int256 price,, uint256 timeStamp, uint80 answeredInRound) = oracle.latestRoundData();
    if (timeStamp == 0) revert IncompleteRound();
    if (answeredInRound < roundID) revert StaleData();
    return price.toUint256();
}

The `answeredInRound < roundID` comparison is meant to detect stale data but according to Chainlink's documentation[^1] `answeredInRound` is deprecated.

**Recommendation:**

Use the more appropriate and up-to-date method of checking for stale data which is comparing the returned `updatedAt` (`timeStamp` in the snippet) timestamp with the current block timestamp. Revert if the difference of times is outside of some reasonable bound.

**Status:**

Confirmed and fixed by shafu0x in [`d6049b3`](https://github.com/DyadStablecoin/contracts-v6/tree/d6049b3fb45529d51f4152907a62b1e21bac8c79).

#### [Core/M-3] Incorrect Oracle Address Configured For Deployment
**Context:** [`src/Parameters.sol#L16`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/Parameters.sol#L16)

**Description:**

For the mainnet deployment of the system the oracle address for the first collateral asset WETH was incorrectly configured to be the [`DNft`](0xDc400bBe0B8B79C07A962EA99a642F5819e3b712) address.

This would lead to any calls depending on the oracle price to revert making those methods unusable e.g. `withdraw`, `liquidate`.

If a user executed the sequence of transactions:
1. `VaultManager.add(id, faultyVault)`
2. `VaultManager.deposit(id, faultyVault, amount)`

Their assets would be stuck, unwithdrawable from the system until the licenser owners intervened by de-licensing the vault to ensure that the `getTotalUsdValue` method would skip calling the faulty vault, un-blocking the function.

**Recommendation:**

Correct the incorrect address and ensure you're not only executing tests in your local foundry testing environment but also running forked mainnet tests. Ideally the process of running your tests against the mainnet configuration should be built into your CI to ensure you're alerted in case you happen to push incorrect code.

**Status:**

Fix of core issue implemented by shafu0x as of [`cf55fcc`](https://github.com/DyadStablecoin/contracts-v6/commit/cf55fcc5e5570b3415b86353fb49d08094034f89). Process recommendations have not yet been implemented.

#### [Core/M-4] Stale Oracle Data Allows the Sytem to Accrue Bad Debt
**Context:** [`src/core/Vault#L90-L99`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/core/Vault.sol#L90-L99)

**Description:**

If the reported oracle data becomes stale for a prolonged period of time it could allow the protocol to start accruing bad debt as the positions of users start becoming undercollateralized.

Note that even if the recommendation for issue [Core/M-2](#CoreM-2-Incorrect-Checking-Of-Stale-Oracle-Data) is implemented it would still only be a mitigation to this issue. This is because while users could no longer borrow based on the stale data the anti-stale-data check would prevent liquidations as well. This means that if the market price of the collateral degrades as the oracle that's meant to report its price is stale and essentially frozen the collateralization ratio of the associated positions could continue falling until they're below 100%.

**Recommendation:**

(1) Add a fallback oracle run by a different that can be queried in the case that the first is stale, this ensures that the price reporting system has redundancy. (2) For more certainty add an emergency liquidation mode that allows a trusted party to liquidate positions if both the main and fallback oracle are stale and the system cannot otherwise be confident of a certain collateralization ratio without being able to value that asset.

**Status:**

Acknowledged.

#### [Core/L-1] Incorrect Decimal Handling in `VaultManager.redeemDyad`

**Context:** [`src/core/VaultManager#L148`](https://github.com/DyadStablecoin/contracts-v6/blob/a8539841ab4a2960dfee523143d9c62d421d40b4/src/core/VaultManager.sol#L148)

**Description:**

Similar to issue [Core/M-1](#CoreM-1-Incorrect-Asset-Price-Denomination) the `VaultManager` contract's `redeemDyad` method does not handle decimals correctly. However due to its main interactions being with the `dyad.burn` and `VaultManager.withdraw` methods its impact is limited to vulnerabilities originating from the `Vault.getUsdValue` method.

Nevertheless this bug on the level of the `redeemDyad` method will degrade the user experience as users suffer unexpected reverts, too small or too large withdrawals. Furthermore the method name is somewhat misleading, the `redeemDyad` function does not independently redeem DYAD for underlying assets but combines the paying down of $x$ DYAD with the withdrawal of $x$ US-Dollars worth of assets. This is more akin to a rebalancing operation, improving the user's collateralization ratio.

**Recommendation:**

Fix the asset amount calculation similar to the recommendation made for issue [Core/M-1](#CoreM-1-Incorrect-Asset-Price-Denomination), taking the assets decimals and the price denomination into account. Furthermore rename the method to better reflect its functionality.

**Status:**

Confirmed and fixed by 0xshafu in [`b0477a0`](https://github.com/DyadStablecoin/contracts-v6/tree/b0477a045e11634ecfd889a6d644448b198c9232).

### Informational Issues

These are issues that have no concrete impact on system functionality. These are purely related to processes and coding best practices. For this reason the issues are laid out as rough bullet points and are more technical in nature. Understanding the following notes may require context of the code.

- `Licenser.sol` does not emit events for licensing changes, can be useful to be able to easily track changes, especially important while the protocol is under centralized control.
- Inconsistent indentation, curly-brace style. It's recommended to use a code formatter such as `forge fmt` or `prettier` as it'll keep the style consistent and maintain it automatically if configured correctly.
- Rename the `licenser` variables in `Dyad.sol` and `VaultManager.sol` so that it's easier to understand which licenser is which when reading the code.
- The modifier `isLicensed` is not used within `VaultManager.sol`;
- Wad constants written with irregular exponents e.g. `15e17` or `8e17`, recommended to always use `e18` combined with the decimal notation: `1.5e18` and `0.8e18`.

## Appendix

### Gas Optimization

Similar to [Informational Issues](#Informational-Issues) the notes will be kept light and in bullet points as they were not the primary focus of this engagement.

- `VaultManager` uses a custom implementation of an address set with a $O(n)$ member removal algorithm instead of a pre-existing library like OpenZeppelin's `EnumerableSet.AddressSet`
- `VaultManager.liquidate` does unnecessary round-trips to retrieve data (both `vaults[id]` and `vault.id2asset(id)` are read 2 and 3 times respectively).
- List of vaults could be stored via SSTORE2/SSTORE3 per id to save gas on reads (assuming `add` & `remove` are expected to be rare, batchable actions).

[^1]: https://docs.chain.link/data-feeds/api-reference#getrounddata, last accessed 2023-12-02 20:03 CET