DYAD - josephdara's results

Lines of code

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

Vulnerability details

Impact

Liquidation can be blocked by the NFT owner. This is because the of a miscalculation in the collateralratio. The collatRatio gets the getTotalUsdValue() of an ID which is then used to calculate the actual collateralratio. However the getTotalUsdValue() add all vaults in both vaults types:

  function getTotalUsdValue(
    uint id
  ) 
    public 
    view
    returns (uint) {
      return getNonKeroseneValue(id) + getKeroseneValue(id);
  }

This is expected except for the fact whereby a vault is present in both vaults[] and vaultsKerosene. If we look at the DeployV2 which is listed in scope, we see that the same vaults are licensed on both sides. In DeployV2:

    kerosineManager.add(address(ethVault));
    kerosineManager.add(address(wstEth));

.
.
.


    vaultLicenser.add(address(ethVault));
    vaultLicenser.add(address(wstEth));
    vaultLicenser.add(address(unboundedKerosineVault));

Hence by adding the same vault to both vaults[] and vaultsKerosene, Liquidations will revert, even though the collateral ratio is adding the same vault twice

Proof of Concept

  function liquidate(
    uint id,
    uint to
  ) 
    external 
      isValidDNft(id)
      isValidDNft(to)
    {
      uint cr = collatRatio(id);
      if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();

Since collatRatio() will return a high value even though there isn't enough collateral, liquidation will revert, the nft owner simply has to call the functions below using the same vault and id:

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

Tools Used

Manual Review, JosephDara

Recommended Mitigation Steps

Prevent additon of a vault to either of the arrays if they are already contained in the other array

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Allowing any user to deposit funds for a particular ID opens up the smart contract to frontrunning, causing withdrawals to fail.

This is due to the check:

    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();

The state variable idToBlockOfLastDeposit[] is updated in the deposit(), hence by depositing 1 wei of collateral in a block, withdrawals can be blocked. Therefore frontrunning a withdrawal transaction with a small deposit causes DOS

Proof of Concept

Anyone can deposit to any ID

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

Tools Used

Manual Review, JosephDara

Recommended Mitigation Steps

Remove the block check or restrict deposits to only the isDNftOwner(id)

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

In the VaultManagerV2, vaults can be added if they are licensed, and removed if they have no funds for an ID. However, addition and removal of vaults can be blocked whenever a Vault which was previously licensed is delicensed.

This is because the withdraw function does not allow withdrawals from unlicensed vaults. Although the check is not done in that function, it is done in getNonKeroseneValue(id) which skips every unlicensed vaults when adding all collateral. Since users can not withdraw collateral from an unlicensed vault, it is impossible to remove the vault from the vaults / vaultsKerosene array using remove() & removeKerosene().

Since the protocol already enforces a strict cap using MAX_VAULTS/MAX_VAULTS_KEROSENE, a user cannot add new licensed vault using add() & addKerosene() if they have their array populated with delicensed and licensed vaults.

One more issue here is, since a malicious user can deposit for any other user, immediately a vault is delicensed, they can simply deposit 1 wei value to all users who currently have that vault in their array, hence prevent removal from the vault from the vaults array for users.

Proof of Concept

The check in the withdrawal & getNonKeroseneValue function

  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(); //@audit getNonKeroseneValue called here
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

  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))) { //@audit excluded all unlicensed vaults
    
  }

Withdrawal fails

  //@audit cannot remove vaults  when it is unlicensed. This is because, you cannot withdraw from unlicensed vaults. Hence id2asset will never be zero

    if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
 

Addition fails here

  function add(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();

Tools Used

Manual Review, JosephDara

Recommended Mitigation Steps

If a vault is delicensed, allow withdrawals from that vaults since it's funds are no more calculated as collateral.

Assessed type

DoS