DYAD - zhaojohnson's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L250-L267

Vulnerability details

Impact

Function getNonKeroseneValue() implementation is incorrect, which could lead to exogenous collateral is less than dyad debt. This is not allowed.

Proof of Concept

In VaultManagerV2, two kinds of assets can be taken as the collateral, exogenous collateral(WETH/WSTETH) and kerosine(unbounded and bounded kerosine). Function getNonKeroseneValue() aims to calculate one NFT position's exogenous collateral's value. Users can add collateral asset through add(). From deploy.v2.s.sol, permited vaults include ethVault, wstETHVault, and unboundedKerosineVault. Users can add unboundedKerosineVault into vaults[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);
  }

When we calculate users' getNonKeroseneValue(), unbounded kerosine assets are calculated as one part of non-kerosine value.

  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))) {
          usdValue = vault.getUsdValue(id);        
        }
        totalUsdValue += usdValue;
      }
      return totalUsdValue;
  }

Tools Used

Manual

Recommended Mitigation Steps

Only weth and wsteth should be calculated for the getNonKeroseneValue() calculation.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L269-L286

Vulnerability details

Impact

Function getKeroseneValue() is expected to calculate kerosene collateral value. Actual calculation includes exogenous collateral value.When we calculate getTotalUsdValue(), some assets might be calculated twice.

Proof of Concept

From Deploy.V2.s.sol, ethVault and wstEth are added into both vaultLicenser and kerosineManager. Then users can add ethVault and wstEth vault into both vaults[id] and vaultsKerosene[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);
  }

When VaultManagerV2 tries to calculate getTotalUsdValue(), value for weth and wsteth will be calculated for twice. Because weth and wstvault exist both in vaults[id] and vaultsKerosene[id], they will be taken as nonkerosene and kerosene at the same time.

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

Tools Used

Manual

Recommended Mitigation Steps

For vaultsKerosene(), we need calculate vaults which belong to vaults, and not belong to vaultsKerosene. In current design, vaults includes all possible vaults and vaultsKerosene include exogenous vaults.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L118-L144

Vulnerability details

Impact

Hacker can block normal user's withdraw() via deposit() function.

Proof of Concept

In VaultManagerV2, we will revert withdraw operation if idToBlockOfLastDeposit[id] == block.number. This aims to prevent flashloan attack. However, this could block normal withdraw case if hacker deposit 0 amount for withdrawer via frontrun.

For example, Alice wants to withdraw some assets. Bob monitors and call one deposit() with alice's NFT id and 0 amount. And idToBlockOfLastDeposit[id] will be updated to the latest.

  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

When Alice withdraws her assets, the transaction will be reverted because of idToBlockOfLastDeposit[id] has already updated to the latest.

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

Tools Used

Manual

Recommended Mitigation Steps

Add one modifier for function deposit(), only NFT owner or owner's delegator can deposit() for the related NFT id.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Function assetPrice() might be reverted when tvl < dyad.totalSupply(), which leads to some key actions' failure, such as withdraw(), liquidate().

Proof of Concept

In VaultManagerV2, kerosine is brought in the system. System will make sure NonKeroseneValue of each NFT position is larger than minted Dyad amount for this NFT position.

The vulnerability is when some exogenous asset price drop down a lot suddenly. The whole system's tvl may be less than dyad.totalSupply(). When liquidators want to liquidate some NFT's position with some kerosine assets, liquidate() will revert because of kerosine vault's assetPrice() will be reverted because of tvl is less than dyad.totalSupply().

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

Tools Used

Manual

Recommended Mitigation Steps

According to doc, Kerosene is thus as valuable as the degree of DYAD’s overcollateralization. When tvl equals to dyad.totalSupply(), the value of kerosene is 0. It makes sense to mark kerosense's value as 0 when tvl is less than dyad.totalSupply().

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

UnboundedKerosineVault & BoundedKerosineVault's assetPrice() will be reverted when tvl < dyad. This would cause withdraw()/liquidate() failure.

Proof of Concept

VaultManagerV2 is one mitigation from VaultManager. From description in the code4rena webpage, The only transaction that needs to be done by the multi-sig after the deployment is licensing the new Vault Manager. We can consider that existing VaultManger will run at the same time.

The vulnerability is that VaultManager and VaultMangerV2 share the same DYAD Token and share to DYAD's totalSupply().

In UnboundedKerosineVault::assetPrice(), if tvl is less than dyad.totalSupply(), assetPrice() will be reverted. And tvl is exogenous collateral USD values in VaultManagerV2. dyad.totalSupply() will contain DYAD minted in VaultMangerV2 and VaultManger. It's easy to meet that condition: tvl is less than dyad.totalSupply().

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

Tools Used

Manual

Recommended Mitigation Steps

Calculated minted DYAD amount for each VaultManager.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Some bounded kerosine profits from liquidation will be locked in contract.

Proof of Concept

When liquidator liquidate one NFT position, all collateral assets will be moved to liquidator. If liquidator has no debt, liquidator can withdraw all profits. However, bounded kerosine vault is not allowed to withdraw. Liquidator may lose some profits.

For example, Alice borrow 100 DYAD via collateral WETH(100$) and bounded kerosine(50$). Now collateral price drops down, Bob wants to liquidate Alice's position. Bob will pay 100 DYAD and get WETH and bounded kerosine. Considering these bounded kerosine are not withdraw-able and transferable, Bob will lose some profit.

Tools Used

Manual

Recommended Mitigation Steps

It's not easy to mitigate this case. Even if we allow liquidator to withdraw related bounded kerosine, it's also unfair for liquidators. Liquidator liquidate bounded kerosine, valued 50$(doubled because of bounded), liquidator will get nearly 25$ value kerosine even if we allow liquidator to withdraw.

Assessed type

Error