DYAD - petro_1912's results

Lines of code

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

Vulnerability details

Impact

collatRatio is an important factor that reflects the ratio of specific DNFT's TVL to minted DYAD. It limits the amount of assets DNFT owner can withdraw from their vaults. However, vaults and vaultsKerosene may contain the same Vault at the same time, so their values may be added twice to getTotalUsdValue and collatRatio. This breaks the collateral mechanism of the DYAD protocol.

Proof of Concept

While adding a vault in vaults array, it only checks if vault has been registered in vaultLicenser and it has already been added.

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

Likewise, while adding a kerosene vault, it only checks if vault has been registered in keroseneManager and it has already been added.

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

And, in Deploy.V2 contract, we can see that ethVault, and wstEth are added in kerosineManager and vaultLicenser.

    KerosineManager kerosineManager = new KerosineManager();
    kerosineManager.add(address(ethVault));
    kerosineManager.add(address(wstEth));
...
    vaultLicenser.add(address(ethVault));
    vaultLicenser.add(address(wstEth));
    vaultLicenser.add(address(unboundedKerosineVault));

If DNFT owner adds ethVault into vaults and vaultsKerosene, then getTotalUsdVaule and collatRatio is twice of real TVL in ethVault. Therefore, owners can mint DYAD with less collateral.

Tools Used

Manual review

Recommended Mitigation Steps

Prevent to store vault together in vaults and vaultsKerosene by checking the vault existence in the two kinds of vaults while adding vault.

  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();          
++  if (vaultsKerosene[id].contains(vault) || !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();
++  if (vaults.contains(vault) || !vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

UnboundedKeroseneVault allows owner to withdraw assets, but owner can't withdraw and redeem assets from that vault if value is much than nonKeroseneValue - dyadMinted.

Proof of Concept

When withdrawing assets from the vault, the DYAD protocol requires that non-kerosene value must not be less than the minted DYAD token. However, the withdraw function does not check whether the withdrawing vault is kerosene or not. If the withdrawing vault is a kind of kerosene, nonKeroseneVaule does not change after the asset is withdrawn from the vault.

  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(); 
L150: if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat(); 
    _vault. Withdraw(id, to, amount); 
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow();
  }

As we can see in L150, nonKeroseneValue is decreased by the value of withdrawal assets regardless of type of vault. If vault is kerosene and value is greater than the difference between non Kerosene Value and minted dyad token, then owner can't withdraw kerosene(unbounded) asset.

Tools Used

Manual review

Recommended Mitigation Steps

  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(); 
++  uint value;
++  if (vaults.contains(vault))
++       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();
  }

Assessed type

Error