DYAD - carlitox477's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L93-L94 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L75 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L88

Vulnerability details

Description

The idea of Kerosine manager is whitelisting/license vaults that are going to be consider to calculate Kerosine intrinsict value, while the idea of Licenser is whitelisting/licensing vaults that will be able to be considered exogenous collateral to mint DYAD.

However, this is not the way that they are used in VaultManagerV2. In this contract, considering that WETH and wstETH vaults are licensed by Licenser contract and Kerosine manager, the same balance associated to an NFT in these vault can be used twice to increase collateral ratio avoiding liquidation of undercollateralized vaults.

Impact

wstETH and WETH vault can be used twice to increase collateral ratio, avoiding liquidation of undercollateralize vaults

POC

1. Bob has an DNFT, associate this NFT to wstETH vault through VaultManagerV2.add(BOB_NFT_ID,wstETH_VAULT).
2. Bob now associate same NFT to wstETH vault through VaultManagerV2.addKerosene(BOB_NFT_ID,wstETH_VAULT). Given it is licensed by keroseneManager this function does not revert
3. Bob deposit 100 USD value in wstETH to wstETH vault by calling VaultManagerV2.deposit(BOB_NFT_ID, wstETH_VAULT,USD_100_VALUE)
4. Vault mint 99 DYAD by calling VaultManagerV2.mint(BOB_NFT_ID, wstETH_VAULT,99 * 10e18). given that collatRatio account for watETH balance twice the transaction does not revert

In addition Bob position cannot be liquidated until wstETH deposit value is reduced to 75 USD, which would leave the protocol totally undercollateralized. This become extremely dangerous in case of an abrupt crash of ETH or stETH price.

Recommended mitigation

The idea of using kerosine manager to also add vault make no sense at all, it should be removed. To track Kerosine and non-Kerosine USD value simply iterate through vaults and check its assets to defien where to add it or not. Here a quick fix to current design, however consider it might also include other bugs. Use it as inspiration for a new contract

Assessed type

Other

Lines of code

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

Vulnerability details

Description

According to docs, code walkthrough and sponsor private messages during contest liquidator should receive rewards for

$\frac{min(20 \%, max(0, cr - 100\%)) + 100\%}{max(cr,100\%)}$

However this is not the case:

  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; //@audit max(cr,100%)
      uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD); // @audit (max(cr,100%) - 100%) * 20% --> Excedent 20%
      uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr); // @audit  ((max(cr,100%) - 100%) * 20% + 100%) / max(cr,100%) 

      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);
  }

The actual reward for a liquidator is a 20% of the remaining associated to the NFT, not a 20% of the 100% collateral. This means that current rewards correspond to:

$\frac{((max(cr,100\%) - 100\%) \times 20\% + 100\%)}{max(cr,100\%)}$

Instead of the desired calculation

Impact

• Liquidators have less incentive to liquidate undercollateralize NFT
• Wrong liquidator reward calculation

$\frac{min(20 \%, max(0, cr - 100\%)) + 100\%}{max(cr,100\%)}$

Recommended mitigation

Modify current codebase to match intended behaviour

//  VaultManagerV2.liquidate
    uint cappedCr               = cr < 1e18 ? 1e18 : cr; // max(cr,100%)
-   uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD); // (max(cr,100%) - 100%) * 20% --> Excedent 20%
+   uint excedentRatio = cappedCr <= 1e18 ? 0 : cappedCr - 1e18;
+   uint liquidationEquityShare = excedentRatio < LIQUIDATION_REWARD ? excedentRatio : LIQUIDATION_REWARD;
    uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr); 

Assessed type

Math