DYAD - falconhoof's results

Lines of code

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

Vulnerability details

Summary

The function getNonKeroseneValue() should only calculate the USD value of exogenous collateral but an incorrect check means instead it calculates the USD value of all collateral, exogenous and endogenous

Vulnerability Details

getNonKeroseneValue() should calculate the USD value of assets stored in non-kerosene vaults, i.e. exogenous collateral vaults, for a given NFT id.

However, the line marked below in getKeroseneValue() incorrectly checks if the vault has been licensed in vaultLicenser and if true adds it's usd value to the calculation but vaults licensed in vaultLicenser are both exogenous and endogenous.

This means the USD value of both endogenous and exogenous collateral associated to an NFT id will be calculated where only the value of the exdogenous collateral is what is required.

    function getNonKeroseneValue(
      uint id
    )
      public
      view
      returns (uint) {
      // SOME CODE

>>>       if (vaultLicenser.isLicensed(address(vault))) {
            usdValue = vault.getUsdValue(id);
            console.log("usdValue Non Kero: ", usdValue);
          }

      // SOME CODE
    }

We can see in the deploy script Deploy.V2.s.sol, both exogenous and endogenous vaults will be licensed in vaultLicenser while only exogenous collateral vaults are licensed in kerosineManager.

    // SOME CODE

    // @audit : exogenous collateral vaults contributing to TVL
    kerosineManager.add(address(ethVault));
    kerosineManager.add(address(wstEth));

    // SOME CODE

    // @audit : all collateral vaults where users can deposit
    vaultLicenser.add(address(ethVault));
    vaultLicenser.add(address(wstEth));
    vaultLicenser.add(address(unboundedKerosineVault));
    // vaultLicenser.add(address(boundedKerosineVault));

    // SOME CODE

Impact

User's non kerosene vault's USD value will be inflated by including kerosene vaults and user's overall collateral ratio will be inflated by double counting kerosene vaults. User's with kerosene deposited in a kerosene vault will be able to mint Dyad for less exogenous collateral than should be possible and therefore Dyad will be backed by less collateral than it should which can lead to it's depegging.

Tools Used

Manual Review Foundry Testing

Recommendations

Update the check to only loop through the exogenous vaults licensed in keroseneManager:

    function getNonKeroseneValue(
      uint id
    )
      public
      view
      returns (uint) {
        uint totalUsdValue;
        uint numberOfVaults = vaults[id].length();
        for (uint i = 0; i < numberOfVaults; i++) {
          Vault vault = Vault(vaults[id].at(i));
          uint usdValue;
 -        if (vaultLicenser.isLicensed(address(vault))) {
 +        if (keroseneManager.isLicensed(address(vault))) {
            usdValue = vault.getUsdValue(id);
          }
          totalUsdValue += usdValue;
        }
        return totalUsdValue;
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Summary

A user can be prevented from calling withdraw() and redeemDyad() by malicious user making a deposit of any amount on their behalf

Vulnerability Details

When a deposit is made on behalf of an NFT id in deposit(), the current block.number is stored against that id in idToBlockOfLastDeposit[id].

This stored value is then used in a check in withdraw(), see below, to prevent the depositing and withdrawing of collateral within the same block.

A malicious actor can use this check to block withdrawls of any collateral associated to an NFT id by front-running the transaction in the same block with a deposit of any amount on behalf of that id.

    function deposit(
      uint    id,
      address vault,
      uint    amount
    )
      external
        isValidDNft(id)
    {
>>>   idToBlockOfLastDeposit[id] = block.number;

      // SOME CODE
    }

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

      // SOME CODE
    }

POC

• Bob calls withdraw() on his NFT id 134
• Alice sees Bob's transaction in the mempool
• Alice deposits 1 wei of wEth on behalf of NFT id 134 in the same block
• Bob's transaction reverts with DepositedInSameBlock() error

Impact

Malicious user can DOS users indefinitely from calling withdraw() or redeemDyad() by consistently front running the transaction in the same block with a deposit on behalf of the victim's NFT id. This affects the availability of core protocol functionality; costing the victim gas as well as locking their funds up.

Tools Used

Manual Review Foundry Testing

Recommendations

Change the deposit() modifier from isValidDNft(id) to isDNftOwner(id) so that deposits can not be made on another user's behalf:

    function deposit(
      uint    id,
      address vault,
      uint    amount
    )
      external
 -      isValidDNft(id)
 +      isDNftOwner(id)
    {
      // SOME CODE
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Summary

Liquidators have no incentive to liquidate users who hold a proportion of their collateral in Kerosene which increases likelihood of bad debt accumulating in the protocol

Vulnerability Details

The Collateral Ratio is used to measure whether the health of a user's position and whether they can be liquidated or not.

It is based on the USD value of a user's holdings of both Exogenous (e.g wEth, wsEth) & Endogenous (e.g. kerosene) collateral and if a user has a ratio less than 1.5e18 they can be liquidated.

The amount of collateral a liquidator receives for liquidating is also derived from this measure.

However users are only paid out from the liquidatee's Exogenous collateral, i.e. vaults stored against a user's NFT id in VaultManagerV2::vaults, as indicated in the function below, and not on their Kerosene collareral.

This means that the larger the proportion of kerosene making up a potential liquidatee's collateral, the smaller the payout becomes for the liqudiator; making liquidations of users with kerosene less likely.

  function liquidate(
    uint id,
    uint to
  )
    external
      isValidDNft(id)
      isValidDNft(to)
    {
      // @audit : cr is based on USD value of Exo and Endo collateral
>>>   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++) {
          // @audit : only exogenous vaults pay out
>>>       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);
  }

POC

• Mary has $50_000 worth of Kerosene
• Mary has 100 wEth deposited which at $1000 is worth $100_000
• This allows Mary to mint 100_000 Dyad for a Collateral Ratio of 150%
  • Of her Collateral Ratio therefore; 100% is Exogenous collateral & 50% is Endogenous
• Then the price of the Endogenous collateral, Kerosene, drops due to holders burning Dyad, TVL reducing or Kero being distributed while the price of the Exogenous collateral (wEth) remains stable
• Mary's new Collateral Ratio is 140% and she can be liquidated
• Nobody will liquidate her though because:
  • They will receive none of her Endogenous collateral
  • For a Collateral Ratio of 140% liquidator only stands to gain 77% of her Exogenous collateral for burning their 100_000 Dyad which would equal 77 wETH
  • However they could otherwise redeem their 100_000 Dyad for 100 wEth or the wstEth equivalent; depending on what they hold.
  • If they had liquidated a user holding only exogenous collateral, e.g. 150 wEth, they would be paid out 150 * .77 = 115.5;
  • It is unlikely anybody would accept such large discounts to liquidate somebody holding kerosene and the higher the proportion of kerosene held the smaller the payout for the liquidator

Liquidation function calculations for cr = 1.4e18 used above: cr = 1.4e18 => cappedCr = 1.4e18 => liquidationEquityShare = (1.4e18 - 1e18) / 0.2e18 / 1e18 => 0.08e18 => liquidationAssetShare = (0.08e18 + 1e18) / 1.4e18 => 0.77e18 => collateral = 100e18 * 0.77e18 / 1e18 => 77e18 wEth

Liquidation function calculations for cr = 1.1e18 used above: cr = 1.1e18 => cappedCr = 1.1e18 => liquidationEquityShare = (1.1e18 - 1e18) / 0.2e18 / 1e18 => 0.02e18 => liquidationAssetShare = (0.02e18 + 1e18) / 1.1e18 => 0.93e18 => collateral = 100e18 * 0.82e18 / 1e18 => 93e18 wEth

Impact

Increase of bad debt in the system and depeg of Dyad.

Tools Used

Manual Review Foundry Testing

Recommendations

Given that users are not entitled to Endogenous collateral; payout amounts should take into account the proportions of Exogenous to Endogenous collateral that the liquidatee is holding and adjust payout accordingly.

Assessed type

Error

Lines of code

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

Vulnerability details

Summary

Checks in addKerosene() and getKeroseneValue(), which should verify whether a vault has been licensed or not, will always revert

Vulnerability Details

Checks in two different functions verify if a kerosene vault has been licensed by querying keroseneManager::isLicensed().

However keroseneManager is the wrong contract to check because the vaults licensed in keroseneManager must be exogenous collateral vaults only as they are used in the TVL calculations in UnboundedKerosineVault::assetPrice().

Therefore, keroseneManager::isLicensed() will always return false where the input is a Kerosene vault.

This is reflected in the deploy script Deploy.V2.s.sol, where only exogenous collateral vaults are licensed in kerosineManager while all vaults users can interact with, endogenous and exogenous, are licensed in vaultLicenser.

    // SOME CODE

    // @audit : exogenous collateral vaults contributing to TVL
    kerosineManager.add(address(ethVault));
    kerosineManager.add(address(wstEth));

    // SOME CODE

    // @audit : all collateral vaults where users can deposit
    vaultLicenser.add(address(ethVault));
    vaultLicenser.add(address(wstEth));
    vaultLicenser.add(address(unboundedKerosineVault));
    // vaultLicenser.add(address(boundedKerosineVault));

    // SOME CODE

addKerosene()

The line marked below in addKerosene() incorrectly checks if the provided vault has been licensed in keroseneManager. This prevents the addition of kerosene vaults by users to the vaultsKerosene mapping against their NFT id.

  function addKerosene(
      uint    id,
      address vault
  )
    external
      isDNftOwner(id)
  {
    // SOME CODE

>>> if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();

    // SOME CODE
  }

getKeroseneValue()

The line marked below in getKeroseneValue() incorrectly checks if the provided kerosene vault has been licensed in keroseneManager. This means only the usd value of the exogenous collateral associated to an NFT id will be calculated where the value of the endogenous collateral is what is actually required.

  function getKeroseneValue(
    uint id
  )
    public
    view
    returns (uint) {
      // SOME CODE

>>>     if (keroseneManager.isLicensed(address(vault))) {
          usdValue = vault.getUsdValue(id);
        }

      // SOME CODE
  }

Impact

User activity with Kerosene vaults is completely blocked because users can't add kerosene vaults to deposit into. Users rewards in the form of Kerosene will not be usable in the protocol; essentially becoming worthless. Only exogenous collateral can be accepted and priced by the protocol, breaking core protocol functionality which should allow users to deposit Kerosene as endogenous collateral.

Tools Used

Manual Review Foundry Testing

Recommendations

Because the vaults licenced in keroseneManager should only be exogenous and the vaults licenced in vaultLicenser should contain both exogenous + endogenous; the easiest thing to do would be to create a new data structure to store only endogenous vaults.

Assessed type

DoS