DYAD - pontifex's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64-L65

Vulnerability details

Impact

Users can receive more dyad than expected by including collateral vaults (ethVault and wstEth) in both the vaults and the vaultsKerosene mappings. This lets receive dyad in 1:1 usd value ration. Also this can prevent the user from liquidation while the collateral usd value not less than 75% of the initial. So the dyad token can be undercollateralized.

Proof of Concept

The DeployV2 contract adds ethVault and wstEth vaults to the KerosineManager contract and to the Licenser contract.

contract DeployV2 is Script, Parameters {
  function run() public returns (Contracts memory) {
//...
    KerosineManager kerosineManager = new KerosineManager();

    kerosineManager.add(address(ethVault));
    kerosineManager.add(address(wstEth));
//...
    vaultLicenser.add(address(ethVault));
    vaultLicenser.add(address(wstEth));

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L62-L94 So users can add ethVault and wstEth vaults as collateral vault in the vaults mapping and as kerosene vault in the vaultsKerosene mapping because these vaults are licensed in both contracts.

  function add(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
    if (!vaults[id].add(vault))            revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

  function addKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
    if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
    if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L67-L91 The mintDyad function checks the usd value of collateral vaults and then the total usd value. The issue lets pass the second check without having any kerosene vaults because value from ethVault and wstEth vaults will be counted in the getTotalUsdValue function twice

  function mintDyad(
    uint    id,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
  {
    uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
    if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();
    dyad.mint(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
    emit MintDyad(id, amount, to);
  }

  function collatRatio(
    uint id
  )
    public 
    view
    returns (uint) {
      uint _dyad = dyad.mintedDyad(address(this), id);
      if (_dyad == 0) return type(uint).max;
      return getTotalUsdValue(id).divWadDown(_dyad);
  }

  function getTotalUsdValue(
    uint id
  ) 
    public 
    view
    returns (uint) {
      return getNonKeroseneValue(id) + getKeroseneValue(id);
  }

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L241-L286

There is the same for the liquidate function.

  function liquidate(
    uint id,
    uint to
  ) 
    external 
      isValidDNft(id)
      isValidDNft(to)
    {
      uint cr = collatRatio(id);
      if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding ethVault and wstEth vaults only in the Licenser contract. It also demands fixing Kerosene vaults can break the protocol functionality issue in the UnboundedKerosineVault contract.

    KerosineManager kerosineManager = new KerosineManager();

-   kerosineManager.add(address(ethVault));
-   kerosineManager.add(address(wstEth));

    vaultManager.setKeroseneManager(kerosineManager);

It can be useful to implement a check in the KerosineManager contract which lets adding only vaults with kerosene token as an asset. The alternative recommendation is checking if the vault asset equals the kerosine token address in the VaultManagerV2.addKerosene function.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L95

Vulnerability details

Impact

Users can receive more dyad than expected by including unboundedKerosineVault vault in the vaults mapping. It will increase the getNonKeroseneValue value by the kerosene token usd value. So users can receive dyad tokens in ratio even less than 1:1 rate in usd value and the dyad token can be undercollateralized.

Proof of Concept

The DeployV2 contract adds the unboundedKerosineVault vault to the Licenser contract.

contract DeployV2 is Script, Parameters {
  function run() public returns (Contracts memory) {
//...
    vaultLicenser.add(address(unboundedKerosineVault));

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L95 So users can add the unboundedKerosineVault vault as a collateral vault in the vaults mapping because the vault is licensed in the Licenser contract.

  function add(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
    if (!vaults[id].add(vault))            revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L67-L78 The mintDyad function checks the usd value of collateral vaults which should not less than the amount of DYAD to be minted but it should not include kerosene tokens. In case the user has enough kerosene (usd value more than collateral usd value) the dyad token can be undercollateralized.

  function mintDyad(
    uint    id,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
  {
    uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
    if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();
    dyad.mint(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
    emit MintDyad(id, amount, to);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Consider not adding kerosene vaults to the Licenser contract but only to the KerosineManager contract. It also demands fixing Kerosene vaults can break the protocol functionality issue in the UnboundedKerosineVault contract.

    vaultLicenser.add(address(ethVault));
    vaultLicenser.add(address(wstEth));
-   vaultLicenser.add(address(unboundedKerosineVault));
-   // vaultLicenser.add(address(boundedKerosineVault));

The alternative recommendation is checking if the vault asset does not equal the kerosine token address in the VaultManagerV2.add function.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L119-L127

Vulnerability details

Impact

An attacker can deposit a low amount, or even zero, of assets to prevent another user withdrawal. Actually attackers can create vaults which are under their control. So they can deposit any amounts of any tokens through the VaultManagerV2.deposit with the result of idToBlockOfLastDeposit[id] updating of the victim for the gas cost only.

Proof of Concept

There is only restriction isValidDNft(id) in the deposit function.

  function deposit(
    uint    id,
    address vault,
    uint    amount
  )
    external
      isValidDNft(id)
  {
    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

So the id owner can't withdraw in the block.number

  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  )
    public
      isDNftOwner(id)
  {
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L134-L143

Tools Used

Manual review

Recommended Mitigation Steps

Consider restriction of donating or/and implementing trusted addresses mapping, who can deposit on behalf of a particular id owner.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L205-L228

Vulnerability details

Impact

Liquidators are not rewarded with Kerosene tokens since only assets from vaults are moved to liquidators. So liquidators receive less than expected and can even have losses in case getNonKeroseneValue(id) < dyad.mintedDyad(address(this), id)).

Proof of Concept

Users add collateral vaults to their dNFT (Note) via the VaultManagerV2.add function and Kerosene vaults via the VaultManagerV2.addKerosene function. The Kerosene vaults are added to vaultsKerosene mapping.

  function add(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
    if (!vaults[id].add(vault))            revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

  function addKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
    if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
    if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L67-L91 In case of liquidation only assets from vaults added to the vaults mapping are moved to the liquidator and all Kerosene tokens stay on the liquidated Note because only vaults mapping are included in the liquidate function as a source of assets.

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

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

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

In the protocol documentation is mentioned The liquidating Note absorbs the liquidated Note’s DYAD balance and captures its collateral, including Kerosene https://dyadstable.notion.site/DYAD-design-outline-v5-ed2d075eb691482d90cae262f822a2ff.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding the vaultsKerosene mapping as a source of assets too.

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

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

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

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L119-L131 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L156-L181 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L184-L202

Vulnerability details

Impact

The protocol allows to create vaults and provide collateral for minting DYAD with no lower limit. As such, multiple low value positions can exist. However, there is no incentive to liquidate low value positions because of gas cost. This can lead to the DYAD undercollaterization.

Proof of Concept

Liquidators liquidate users for the profit they can make. If there is no profit to be made then there will be no one to call the liquidate function. For example a position could exist with a relatively low collateral value. This user is undercollateralized and must be liquidated in order to ensure that the protocol remains overcollateralized. If a liquidator wishes to liquidate this user, they will first need to mint some DYAD which involves gas cost. Because the value of the collateral is low, after gas costs, liquidators will not make a profit liquidating this user. In the end these low value positions will never get liquidated, leaving the protocol with bad debt and can even cause the protocol to be undercollateralized with enough small value accounts being underwater.

Tools Used

Manual review

Recommended Mitigation Steps

A potential fix would be to set a minimum threshold for collateral value which has to be exceeded in order for a user to mint DYAD.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L50-L61

Vulnerability details

Impact

Users can't use kerosene vaults in the usd total value calculation because it breaks the protocol core functionality due to the UnboundedKerosineVault.assetPrice() function invokes itself. But it can't cause asset locking since users are able to burn dyad and bypass the function while withdrawal.

Proof of Concept

Users can use kerosene tokens to overcollaterize their position when dyad minting. It can be counted as part of total usd value in the collatRatio function.

  function mintDyad(
    uint    id,
    uint    amount,
    address to
  )
    external
      isDNftOwner(id)
  {
    uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
    if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();
    dyad.mint(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();
    emit MintDyad(id, amount, to);
  }

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L156-L169 The kerosene vault has to be added to the KerosineManager contract before users will be able to add it to the vaultsKerosene mapping.

function addKerosene(
      uint    id,
      address vault
  )
    external
      isDNftOwner(id)
  {
    if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
    if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
    if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L80-L91 But when a kerosine vault is added to the vaultsKerosene mapping for a dNFT the getKeroseneValue becomes DoSed due to self invoke in the UnboundedKerosineVault.assetPrice() function.

  function assetPrice()
    public
    view
    override
    returns (uint) {
      uint tvl;
      address[] memory vaults = kerosineManager.getVaults();
      uint numberOfVaults = vaults.length;
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[i]);
        tvl += vault.asset().balanceOf(address(vault))
                * vault.assetPrice() * 1e18

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L56-L61

Tools Used

Manual review

Recommended Mitigation Steps

Consider receiving collateral vaults from the Licenser contract instead of the KerosineManager contract. It also demands a similar with the KerosineManager implementation of the Licenser contract. The alternative recommendation is skipping the vault asset when it equals the kerosine token address in the UnboundedKerosineVault.assetPrice() function.

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import {KerosineVault}        from "./Vault.kerosine.sol";
import {IVaultManager}        from "../interfaces/IVaultManager.sol";
import {Vault}                from "./Vault.sol";
import {Dyad}                 from "./Dyad.sol";
-import {KerosineManager}      from "./KerosineManager.sol";
+import {Licenser}      from "./Licenser.sol";
import {BoundedKerosineVault} from "./Vault.kerosine.bounded.sol";
import {KerosineDenominator}  from "../staking/KerosineDenominator.sol";

import {ERC20}           from "@solmate/src/tokens/ERC20.sol";
import {SafeTransferLib} from "@solmate/src/utils/SafeTransferLib.sol";

contract UnboundedKerosineVault is KerosineVault {
  using SafeTransferLib for ERC20;

  Dyad                 public immutable dyad;
  KerosineDenominator  public kerosineDenominator;

  constructor(
      IVaultManager   _vaultManager,
      ERC20           _asset, 
      Dyad            _dyad, 
-     KerosineManager _kerosineManager
+     Licenser _licenser
- ) KerosineVault(_vaultManager, _asset, _kerosineManager) {
+ ) KerosineVault(_vaultManager, _asset, _licenser) {
      dyad = _dyad;
  }

<...>

  function assetPrice() 
    public 
    view 
    override
    returns (uint) {
      uint tvl;
-     address[] memory vaults = kerosineManager.getVaults();
+     address[] memory vaults = licenser.getVaults();
      uint numberOfVaults = vaults.length;
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[i]);
        tvl += vault.asset().balanceOf(address(vault)) 
                * vault.assetPrice() * 1e18
                / (10**vault.asset().decimals()) 
                / (10**vault.oracle().decimals());
      }
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }
}

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import {KerosineVault}          from "./Vault.kerosine.sol";
import {IVaultManager}          from "../interfaces/IVaultManager.sol";
import {Dyad}                   from "./Dyad.sol";
-import {KerosineManager}      from "./KerosineManager.sol";
+import {Licenser}      from "./Licenser.sol";
import {UnboundedKerosineVault} from "./Vault.kerosine.unbounded.sol";

import {ERC20} from "@solmate/src/tokens/ERC20.sol";

contract BoundedKerosineVault is KerosineVault {
  error NotWithdrawable(uint id, address to, uint amount);

  UnboundedKerosineVault public unboundedKerosineVault;

  constructor(
    IVaultManager   _vaultManager,
    ERC20           _asset, 
-     KerosineManager _kerosineManager
+     Licenser _licenser
- ) KerosineVault(_vaultManager, _asset, _kerosineManager) {}
+ ) KerosineVault(_vaultManager, _asset, _licenser) {}

<...>
}

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import {IVaultManager}   from "../interfaces/IVaultManager.sol";
-import {KerosineManager}      from "./KerosineManager.sol";
+import {Licenser}      from "./Licenser.sol";
import {IVault}          from "../interfaces/IVault.sol";

import {SafeTransferLib} from "@solmate/src/utils/SafeTransferLib.sol";
import {ERC20}           from "@solmate/src/tokens/ERC20.sol";
import {Owned}           from "@solmate/src/auth/Owned.sol";

abstract contract KerosineVault is IVault, Owned(msg.sender) {
  using SafeTransferLib for ERC20;

  IVaultManager   public immutable vaultManager;
  ERC20           public immutable asset;
- KerosineManager public immutable kerosineManager;
+ Licenser public immutable licenser;

<...>

  constructor(
    IVaultManager   _vaultManager,
    ERC20           _asset, 
-     KerosineManager _kerosineManager
+     Licenser _licenser
  ) {
    vaultManager    = _vaultManager;
    asset           = _asset;
-   kerosineManager = _kerosineManager;
+   licenser = _licenser;
  }

<...>
}

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L50-L68

Vulnerability details

Impact

Kerosene token price can be manipulated via relatively small asset withdrawal to push particular positions under MIN_COLLATERIZATION_RATIO for their liquidation.

Proof of Concept

A malicious liquidator (dNFT owner) can keep in controlled vault assets without minting dyad token. The usd value of these assets can consist of a sufficient percentage of the difference between non kerosene usd value and dyad total supply.

  function assetPrice()
    public
    view
    override
    returns (uint) {
      uint tvl;
      address[] memory vaults = kerosineManager.getVaults();
      uint numberOfVaults = vaults.length;
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[i]);
        tvl += vault.asset().balanceOf(address(vault))
                * vault.assetPrice() * 1e18
                / (10**vault.asset().decimals())
                / (10**vault.oracle().decimals());
      }
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }

In case of these assets instant withdrawal the kerosene price will be decreased by the same percent. For the boundedKerosineVault the decrease will be doubled. The current dyad.totalSupply() is approx 622,967.4 DYAD (https://etherscan.io/token/0x305b58c5f6b5b6606fb13edd11fbdd5e532d5a26#readContract). The min overcollateralized rate is 1.5. The non kerosene usd value except for the attacker assets can be 1,000,000.0 USD. So the difference is approx 400,000.0 USD. Assume the attacker keeps 200,000.0 USD in assets. So to push the kerosene price to 1% it is enough to withdraw 6,000.0 USD in asset equivalents. The attacker can choose users with relatively low collateralization ratio and sufficient amount of kerosene token amounts, push the kerosene price down and liquidate victims positions.

Tools Used

Manual review

Recommended Mitigation Steps

Consider tracking time weighted average kerosene price for the position health estimation.

Assessed type

Other