DYAD - koo's results

Lines of code

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

Vulnerability details

Impact

In vaultManagerV2.sol,any one can call deposit function to deposit any amount asset to the vault. However, withdraw function will be revert in [1] once the deposit function was frontrun with same id in the same block. Thus Attacker can frontrun a deposit request with amount 0 to block the withdraw reqeust. And if the attacker want, no one can withdraw funds from the vault.

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

Proof of Concept

1. Attacker observed a withdraw request.
2. Attacker frontrun a deposit request which has the same id with the previous withdraw requests and with 0 amount.
3. The withdraw request will be reverted and attacker only pay some gas fees.

Tools Used

VS

Recommended Mitigation Steps

Set a whitelist for deposit function. The nft owner can decide who can call the deposit function.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Withdraw function should guarantee the usd value of NonKerosene asset is higher than dyadminted. Thus we have the check in [1]. However, if user want to withdraw Kerosene asset. We should only check

if(getNonKeroseneValue(id) < dyadMinted) revert NotEnoughExoCollat();

rather than

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

  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();  -------------------[1]
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

Proof of Concept

1. Assume a nft owner has $10 NonKerosene asset and $8 Kerosene asset. They mint $9 value dyad.
2. The owner want to withdraw $3 Kerosene asset.
3. However, with this check in [1]. The withdraw operator will not success.

Tools Used

vs

Recommended Mitigation Steps

If the user withdraw Kerosene asset. The check in [1] should be

if(getNonKeroseneValue(id) < dyadMinted) revert NotEnoughExoCollat();

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The liquadate function in VaultManagerV2.sol only transfer the vaults to the liquidator in [1]. However, it ignore the vaultsKerosene. Which makes liquidator get less asset in liquidation than they should get.

  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(); ---------------[1]
      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);
  }

Proof of Concept

1. Assume a nft owner has $10 NonKerosene and $8 Kerosene and they mint $9 dyad.
2. With time goes by ,now NonKerosene drop to $4 and the collatRatio is 133.3% and they should be liquadate.
3. A liquidator use $9 dyad liquidate the nft. They should get ((133.3%-100%)*20%+100%)/133.3% =80% percent of the asset.
4. However, the asset in vaultsKerosene won't transfer to the liquidator after liquidation. Thus they get $3.2 value NonKerosene asset but lost the $6.4 value Kerosene asset

Tools Used

vs

Recommended Mitigation Steps

Add a for loop to transfer the asset in vaultsKerosene.

Assessed type

Context