DYAD - Dudex_2004's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L150

Vulnerability details

The user deposits the collateral tokens into the system to mint Dyad tokens and these collateral tokens are currently wETH and wstETH, and will soon include LSTs and LRTs, other types of yield-bearing collateral, as well as Kerosene. The mintDyad function mints Dyad token to users at their 150% collateral ratio while the withdraw function sends the user deposited collateral to receiver which could be any collateral token mentioned above. However, the function only checks that the existing collateral token should be nonKerosene tokens for the dNFT id.

  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
    uint dyadMinted = dyad.mintedDyad(address(this), id);
    Vault _vault = Vault(vault);
    uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() 
                  / 10**_vault.asset().decimals();
@>  if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

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

So the problem is here that if the user has only deposited Kerosene tokens to mind Dyad tokens instead of nonKerosene tokens then the withdraw and mintDyad function will not be executed because these function checks that the exogeneous collateral should be deposited. Cause user only deposited Kerosene tokens these functions will revert.

Impact

User will not be able to withdraw his deposited kerosene tokens. User will not be able to mint Dyad tokens.

Proof of Concept

1. User deposits Kerosene tokens.
2. The value of the user collateral has increased significantly since the initial deposit, the user may decide to withdraw it to realize profits.
3. Now it is impossible for user to withdraw Kerosene tokens.

Or

1. User deposits kerosene tokens to mint Dyad token.
2. It is impossible to mint Dyad.

Tools Used

Manual review

Recommended Mitigation Steps

Add getTotalUsdValue(id) instead of getNonKeroseneValue(id) in both functions and change the revert name as NotEnoughCollat.

-    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
+    if (getTotalUsdValue(id) - value < dyadMinted) revert NotEnoughExoCollat();

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The attacker could perform asset price manipulation to make the apparent value of an asset to be much higher or much lower than the true value of the asset. Following are some risks of asset price manipulation:

An attacker can increase the value of dyad and the kerosine tokens by increasing the supply of it into the system. An attacker can decrease the value of dyad by redeeming the dyad from the system.

Proof of Concept

The assetPrice function within the Vault.kerosine.unbounded.sol contract is vulnerable to asset price manipulation because the price can be increased or decreased by inflating the totalSupply of dyad or kerosine tokens.

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

The Vault.kerosine.unbounded.sol inherits from KerosineVault and allow us to deposit and withdraw the kerosines as well as it calculates the getUsdValue according to the assetPrice.

 function getUsdValue(
    uint id
  )
    public
    view 
    returns (uint) {
      return id2asset[id] * assetPrice() / 1e8;
  }

But the problem is here that this assetPrice is inflated by the attacker by targeting the totalSupply of dyad or kerosine tokens. Which will effect the numerater or denominator and calculate assetPrice accordingly.

uint numerator   = tvl - dyad.totalSupply();

function denominator() external view returns (uint) {
    // @dev: We subtract all the Kerosene in the multi-sig.
    //       We are aware that this is not a great solution. That is
    //       why we can switch out Denominator contracts.
    return kerosine.totalSupply() - kerosine.balanceOf(MAINNET_OWNER);
  } 

Tools Used

Manual review.

Recommended Mitigation Steps

Consider implementing TWAP so that the price of asset cannot be inflated or deflated.

Assessed type

Other