DYAD - Honour's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L88 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/KerosineManager.sol#L44

Vulnerability details

Impact

keroseneManager verifies/licenses exogenous/non-kerosene vaults rather than keroseneVaults, as a result

1. users will be unable to add keroseneVaults to their DNft positions
2. users will be able to add the same non-kerosene vault twice(as vault and vaultKerosene) allowing the same collateral to be duplicated can break the TVL > DYAD invariant

Proof of Concept

keroseneManager::isLicensed determines if a keroseneVault is licensed or not by checking if it exists in the addressSet vaults https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/KerosineManager.sol#L44 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L88, However the addressSet vaults stores only non-kerosene/exogenous vaults that are used calculate the TVL of Dyad and the price of Kerosene https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64-L65. So when adding vaults with vaultManagerV2::addKerosene users will be unable to add keroseneVaults and would instead be able to add non-kerosene vaults. This means that users will be able to add the same exogenous vault as both kerosene and non-kerosene vault and duplicating their collateral

Tools Used

Manual Review

Recommendation

keroseneMAnager should inherit from the licenser to add licensing functions , the add/remove functions on the keroseneManager should be renamed to distingush them from that of the licenser and the isLicensed function should be removed as it's provided by the licenser

// src/core/KeroseneManager.sol

contract KerosineManager is Licenser {//note licenser is owned already
  error TooManyVaults();
  error VaultAlreadyAdded();
  error VaultNotFound();

  using EnumerableSet for EnumerableSet.AddressSet;

  uint public constant MAX_VAULTS = 10;

  EnumerableSet.AddressSet private vaults;

  function addTVLVault(
    address vault
  ) 
    external 
      onlyOwner
  {
    if (vaults.length() >= MAX_VAULTS) revert TooManyVaults();
    if (!vaults.add(vault))            revert VaultAlreadyAdded();
  }

  function removeTVLVault(
    address vault
  ) 
    external 
      onlyOwner
  {
    if (!vaults.remove(vault)) revert VaultNotFound();
  }

  function getVaults() 
    external 
    view 
    returns (address[] memory) {
      return vaults.values();
  }

}

Deploy script should be updated as well with said functions

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L95-L96 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L75

Vulnerability details

Impact

Users will be able to add the same kerosene vault twice(as vault and vaultKerosene) allowing the same collateral to be duplicated and breaking the TVL > DYAD invariant ,because undercollateralized positions will still be considered healthy

Proof of Concept

Kerosene vaults are added to the vaultLicenser in the deploy script https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L95-L96, the vaultLicenser is used verify licensed non-kerosene vaults in the VaultManagerV2 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L75 this means that users can add the same kerosene vault to VaultManagerV2::vaults and VaultManagerV2::vaultsKerosene , effectively allowing Dyad to be backed entirely by kerosene instead of exogenous collateral

Tools Used

Manual Review

Recommendation

Kerosene vaults should not be added to the vaultLicenser

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

A malicious actor can prevent/delay a DNFT owner from withdrawing their collateral for a period of time

Proof of Concept

VaultManagerV2::withdraw reverts if a deposit to the same DNFT has been made in the same block https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L143. Whilst the withdraw function can only be called by the DNFT owner, the deposit function can be called by anyone as long as its to a valid DNFT https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L125. In specific cases , an example is if a sufficiently large amount of collateral is about to be withdrawn that might affect the price of kerosene ,a malicious actor might be incentivized to delay the withdrawal of collateral for period of time to prevent a loss(perhaps selling their kerosene) or to profit from the price change by front-running the withdrawal to deposit a sufficiently small amount of collateral to the DNFT

Tools Used

Manual Review

Recommendation

Since this was supposed to mitigate flashLoan attacks , use of transient storage will be more appropriate

// src/core/VaultManagerV2.sol

pragma solidity =0.8.24; 

contract VaultManagerV2 {
    error DepositedInSameBlock();

    modifier flashlock() {
        assembly {
            tstore(0, 1)
        }
        _;
    }

    modifier whenNotflashLocked() {
        uint isLocked;
        assembly {
            isLocked := tload(0)
        }
        if (isLocked == 1) revert DepositedInSameBlock();
        _;
    }
    //...code...

    function deposit(uint id, address vault, uint amount) external isValidDNft(id) flashLock {
      
    }

    function withdraw(uint id, address vault, uint amount, address to) public isDNftOwner(id)  whenNotflashLocked {
    }
}

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

Users will be unable to withdraw/redeem kerosene deposited into the vault

Proof of Concept

When withdrawing collateral from the VaultManagerV2 the value(in usd) of the amount of collateral is calculated as https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L146-L149

uint value = (amount * _vault.assetPrice() * 1e18) /
            10 ** _vault.oracle().decimals() /
            10 ** _vault.asset().decimals();

The issue is that kerosene vaults don't use oracles so the transction reverts, preventing kerosene withdrawals. Same calulation happens in the VaultManagerV2::redeemDyad function https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L195-L198

Tools Used

Manual Review

Recommendation

The oracle decimals should be calculated if the vault is not kerosene vault other it's set to 8 which is the decimal precision used by kerosene vaults

// src/core/VaultManagerV2.sol
uint oracleDecimal = kereseneManager.isLicensed(vault) ? 8 : _vault.oracle().decimals();

uint value = (amount * _vault.assetPrice() * 1e18) /
            10 ** oracleDecimal /
            10 ** _vault.asset().decimals();

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

Users may not be able to withdraw kerosene from their vault even if they have a healthy position

Proof of Concept

VaultManagerV2::withdraw reverts if the value the token to be withdrawn causes the value of non-kerosene collateral to fall below the Dyad minted by the DNft position https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L150. The issue is that the check is also done even when withdrawing kerosene, so if the value of kerosene to be withdrawn minus the value of non-kerosene collateral to fall below the Dyad minted the transaction reverts with NotEoughExoCollat() ,which is not the case.

Tools Used

Manual Review

Recommendation

check should be skipped when withdrawing from a kerosene vault.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

TVL > DYAD invariant can be broken

Proof of Concept

VaultManagerV2::liquidate only checks the overall collateral ratio of the DNft to determine if its liquidatable or not https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L214 ,however the tvl(total exogenous collateral) of a position might be less than the dyad minted by the DNft(TVL(id) < DYAD(id)) if the price of the exogenous token drops, but if a sufficient amount of kerosene is available the collateral ratio will remain above 1.5, in such a case the position has undercollateralized exogenous collateral but is not liquidatable

Tools Used

Manual Review

Recommendation

A DNft position should be liquidatable if total exogenous collateral is less than the dyad minted regardless of the collateral ratio to avoid violating the TVL > DYAD invariant

// src/core/VaultManagerV2.sol

-214 if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
+214 if (cr >= MIN_COLLATERIZATION_RATIO && getNonKeroseneValue(id) >= dyad.mintedDyad(address(this), id)) revert CrTooHigh();

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

liquidator might not receive full compensation for burned Dyad token or liquidation reward

Proof of Concept

VaultManagerV2::liquidate only distributes non kerosene collateral to the DNft provided by the liquidator https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L221. However because the minimum value of exogenous collateral required is equal to Dyad minted by the DNft , the amount of exogenous collateral available during liquidation might be less than the Dyad minted while the liquidator burns an equivalent of the Dyad minted by the DNft. For example , a healthy DNft position of 100Dyad backed by 0.0285WETH(100usd @3500/ETH) and 50usd worth of kerosene Collateral ratio=1.5, if WETH falls to 3400usd the position becomes liquidatable because 0.0285WETH is now 97usd and CR < 1.5. The liquidator burns 100Dyad but recieves 97usd(for simplicity , in reality the liquidator recieves less due to how the liquidityAssetShare is calculated) which doesn't even cover the burned Dyad without including the liquidation reward.

Tools Used

Manual Review

Recommendation

Kerosene should be distributed as well on liquidation

// src/core/VaultManagerV2.sol

function liquidate(
        uint id,
        uint to
    ) external isValidDNft(id) isValidDNft(to) {
      //...code...

      uint numberOfVaults = vaults[id].length();
        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);
        }

        numberOfVaults = vaultsKerosene[id].length();
        for (uint i = 0; i < numberOfVaults; i++) {
            Vault vault = Vault(vaultsKerosene[id].at(i));
            uint collateral = vault.id2asset(id).mulWadUp(
                liquidationAssetShare
            );
            vault.move(id, to, collateral);
        }
    }

Assessed type

Other