DYAD - SpicyMeatball's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L89 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L76 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L93-L94

Vulnerability details

Impact

It's possible to double position collateral ratio simply by adding the same vault to another list: vaults or vaultsKerosene. Such positions can't be liquidated even if it's true collateral ratio is less than 150% leading to DYAD depegging.

Proof of Concept

In the deploy script, when ETH and stETH vaults are deployed they are added both in vaultLicenser and keroseneManager.

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

    ---SNIP---

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

When a user deposits in one of the vaults and wants to mint DYAD tokens, he needs to add this vault into his Note token with add function, alternatively he can add vault with addKerosene.

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

If the vault is added into vaults list, it's locked value will be included in getNonKeroseneValue, which is used to calculate exogenous collateral. The value locked in vaultsKerosene is used in collateral ratio calculation along with the values in vaults.

  function collatRatio(
    uint id
  )
    public 
    view
    returns (uint) {
      uint _dyad = dyad.mintedDyad(address(this), id);
      if (_dyad == 0) return type(uint).max;
>>    return getTotalUsdValue(id).divWadDown(_dyad);
  }

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

This creates an issue where a user can add the same vault into both lists, doubling his TVL and collateral ratio.

Check this coded POC, Deploy.V2.s.sol used as a setup with some changes: oracles (price 3000e8), dyad and dnft are mocks, run in forked environment.

  function test_DoubleColl() public {
      address alice = address(0xa11ce);
      deal(address(weth), alice, 100e18);
      uint256 id = dNft.mintNft{value: 1 ether}(alice);

      vm.startPrank(alice);
      weth.approve(address(vaultManager), type(uint256).max);
      vaultManager.add(id, address(ethVault));
      vaultManager.deposit(id, address(ethVault), 10e18);
      vaultManager.mintDyad(id, 20000e18, alice);

      uint256 tvl = vaultManager.getTotalUsdValue(id);
      uint256 cr = vaultManager.collatRatio(id);
      console.log("TVL CR: ", tvl, cr);

      vaultManager.addKerosene(id, address(ethVault));
      tvl = vaultManager.getTotalUsdValue(id);
      cr = vaultManager.collatRatio(id);
      console.log("TVL CR: ", tvl, cr);
  }

At first Alice had TVL = 30000, DYAD = 20000, CR = 1.5, after ethVault was added to vaultsKerosene her TVL = 60000, CR = 3.

Tools Used

Foundry

Recommended Mitigation Steps

Make sure to check whether the vault added to vaults is a non-kerosene vault, and that the vault added to vaultsKerosene is a kerosene vault.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Liquidators may not have enough DYAD to close large underwater loans.

Proof of Concept

To close a DYAD loan with a collateral ratio (CR) < 150%, a liquidator must burn his DYAD tokens in an amount equal to the loan's debt.

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

A situation may arise where a loan's DYAD amount is too big for other users to close. Users may try to use a flash loan but the protocol is not particular flash loan friendly and users will encounter difficulties:

1. the flash loan receiver is unable to withdraw in the same block, and thus must rely on collateral received from liquidating the loan and moved to to id. It may be not enough to repay the flash loan, especially if CR is 1 or lower;
2. to obtain the needed DYAD amount, the liquidator needs to deposit at least x1.5 times it's value in asset tokens, necessitating a larger flash loan that will be more challenging to repay, see point 1.

In summary, liquidators could face significant hurdles in closing large loans, even when attempting to utilize a flash loan.

Tools Used

Manual review

Recommended Mitigation Steps

Consider allowing partial liquidations to ease a strain on a single liquidator and incentivize smaller depositors to close large debts.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Users will be unable to redeem/withdraw their assets.

Proof of Concept

In the VaultManagerV2.sol contract, the deposit function allows anyone to deposit any asset into the specified id. When the deposit function is executed block number is saved.

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

This is done to protect the protocol from flash loans, so users wouldn't withdraw tokens in the same transaction.

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

However, this check can be exploited to front-run withdrawal transaction with a dust amount deposit into the id to revert it and deny withdrawal in this block. It can be repeated multiple times, it doesn't even need to be a deposit into a licensed vault, as any contract would suffice.

Tools Used

Manual review

Recommended Mitigation Steps

• check that the vault a user deposits in is licensed,
• add a minimal deposit amount check

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Assets that were added into vaultsKerosene vault can't be withdrawn.

Proof of Concept

The issue occurs during the withdraw function call, where there is a check that the locked exocollateral is greater than the minted DYAD amount.

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

If the vault that is user withdraws from is stored in the vaultsKerosene instead of vaults, getNonKeroseneValue will return 0, resulting in a transaction revert due to underflow.

Coded POC, Deploy.V2.s.sol used as a setup with some changes: oracles (price 3000e8), dyad and dnft are mocks, run in forked environment.

  function test_NoWithdraw() public {
      address alice = address(0xa11ce);
      deal(address(weth), alice, 10e18);
      uint256 aliceId = dNft.mintNft{value: 1 ether}(alice);

      vm.startPrank(alice);
      weth.approve(address(vaultManager), type(uint256).max);
      vaultManager.addKerosene(aliceId, address(ethVault));
      
      vaultManager.deposit(aliceId, address(ethVault), 10e18);
      vm.roll(1);
      // revert with underflow
      vaultManager.withdraw(aliceId, address(ethVault), 10e18, alice);
  }

Marked this issue as High, given that there are separate allow lists for kerosene and non-kerosene vaults, and a user may be unable to add the vault to the vaults list to successfully withdraw his assets.

Tools Used

Foundry

Recommended Mitigation Steps

In the event of a user withdrawing from a kerosene-vault aka vault that doesn't participate in minting tokens, we can potentially skip the exocollateral check.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L65 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L74

Vulnerability details

Impact

All calls to kerosene vaults that involve the assetPrice() function will revert, including liquidate, withdraw, redeem etc. The main impact is that users may use this to their advantage and avoid liquidation simply by adding kerosene vaults to their id.

Proof of Concept

Because DYAD has already been live for some time, there is non-zero DYAD token total supply in circulation, mainly from operations with previous generation of vaults. As of 24/04 DYAD total supply is 622967400000000000000000, which is roughly $622.967,4.

If we look at the script, we see that the DYAD team deploys fresh empty vaults,

    UnboundedKerosineVault unboundedKerosineVault = new UnboundedKerosineVault(
      vaultManager,
      Kerosine(MAINNET_KEROSENE), 
>>    Dyad    (MAINNET_DYAD),
      kerosineManager
    );

this will cause a problem because kerosene token calculates it's price using balances inside the vaults and DYAD total supply.

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

Until the TVL in this new vaults reaches $622.967, this call will always revert. Loaners can use it to their advantage by adding kerosene vaults to their ids and become invincible to liquidations.

  function liquidate(
    uint id,
    uint to
  ) 
    external 
      isValidDNft(id)
      isValidDNft(to)
    {
>>    uint cr = collatRatio(id); // calls vault.assetPrice() inside

Here is the coded POC, I used Deploy.V2.s.sol script with mock aggregators (price 3000e8) and dNft for a setup, run in the forked environment.

  function test_CantTouchThis() public {
      address alice = address(0xa11ce);
      address bob = address(0xb0b);
      deal(address(weth), alice, 100e18);
      deal(address(weth), bob, 100e18);
      uint256 aliceId = dNft.mintNft{value: 1 ether}(alice);
      uint256 bobId = dNft.mintNft{value: 1 ether}(bob);

      vm.startPrank(alice);
      weth.approve(address(vaultManager), type(uint256).max);
      vaultManager.add(aliceId, address(ethVault));
      
      vaultManager.deposit(aliceId, address(ethVault), 10e18);
      vaultManager.mintDyad(aliceId, 20000e18, alice);
      // Alice adds empty kerosine vault, making her loan invulnerable to liquidations
      // because vault is empty it can be removed anytime
      vaultManager.add(aliceId, address(unboundedKerosineVault));
      vm.stopPrank();
      
      vm.startPrank(bob);
      weth.approve(address(vaultManager), type(uint256).max);
      vaultManager.add(bobId, address(ethVault));
      vaultManager.deposit(bobId, address(ethVault), 100e18);
      vaultManager.mintDyad(bobId, 20000e18, bob);

      // the price has changed, try liquidate Alice 
      wethOracle.setPrice(2900e8);
      // will revert with underflow
      vaultManager.liquidate(aliceId, bobId);
  }

Tools Used

Foundry

Recommended Mitigation Steps

We can remove kerosene vaults from the licenser until other vaults collect enough liquidity, but that could take a long time. Another solution is to use virtual supply for DYAD that will start from 0 and accumulate with each mint in the Vault manager v2.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L65

Vulnerability details

Impact

If the price of assets in the Kerosine vaults experiences a significant drop, it can lead to the assetPrice function underflow, rendering all operations involving Kerosine vaults (liquidation, withdrawals, etc.) unavailable.

Note: This one is different from the similar bug related to already minted DYAD total supply, as the root of the underflow is the price movement

Proof of Concept

The price of Kerosine is calculated using TVL of all vaults and the DYAD total supply.

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

The numerator tvl - dyad.totalSupply() is vulnerable to sudden price movements as it is based on the price of assets in vaults, If the value of assets in the vaults drops significantly, the TVL may become less than the DYAD supply, resulting in an underflow of the assetPrice function. Since collateral calculation depends on this function, all operations involving Kerosine vaults would revert, including liquidations.

This vulnerability poses a high risk, as it would prevent liquidators from stabilizing the protocol.

Tools Used

Foundry

Recommended Mitigation Steps

If TVL is less than DYAD supply it's possible to return the price of Kerosine as 0.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Small loans may not be liqiuidated, which can lead to depegging of the protocol.

Proof of Concept

The current implementation of the Vault manager has no minimal loan limit. This could lead to an issue when there would be little motivation for liquidators to close small underwater positions. The gas costs involved in doing so may not justify the collateral reward they would receive as an incentive.

Currently there are 646 dNft tokens, let's say it costs $10 in gas to liquidate someone. In order to break even, the liquidator must earn at least $10 + DYAD value he burned in received collateral assets. Assume the best case scenario in which collateral ratio of the target is ~1.4:

• Alice minted 100 DYAD with 0.05 ETH at ETH/USD = 3000;
• price goes to 2900;
• Bob liquidates Alice with his 100 DYAD and receives ~$110 in ETH
• the profit is zero = 110 - 100 - 10

You can check the numbers in POC below, as always Deploy.V2.s.sol was used as a setup with mock oracles, nft and dyad, run in forked mainnet.

  function test_Small() public {
      address alice = address(0xa11ce);
      address bob = address(0xb0b);
      deal(address(weth), alice, 100e18);
      deal(address(kerosene), alice, 100000e18);
      deal(address(weth), bob, 1000e18);
      uint256 aliceId = dNft.mintNft{value: 1 ether}(alice);
      uint256 bobId = dNft.mintNft{value: 1 ether}(bob);

      vm.startPrank(alice);
      weth.approve(address(vaultManager), type(uint256).max);
      vaultManager.add(aliceId, address(ethVault));
      
      vaultManager.deposit(aliceId, address(ethVault), 0.05e18);
      vaultManager.mintDyad(aliceId, 100e18, alice);
      vm.stopPrank();
      
      vm.startPrank(bob);
      weth.approve(address(vaultManager), type(uint256).max);
      vaultManager.add(bobId, address(ethVault));
      vaultManager.deposit(bobId, address(ethVault), 100e18);
      vaultManager.mintDyad(bobId, 20000e18, bob);

      // the price has changed, try liquidate Alice 
      wethOracle.setPrice(2900e8);
      console.log("BOB WETH0: ", ethVault.id2asset(bobId));
      console.log("BOB DYAD0: ", dyad.balanceOf(bob));

      vaultManager.liquidate(aliceId, bobId);
      console.log("BOB WETH1: ", ethVault.id2asset(bobId));
      console.log("BOB DYAD1: ", dyad.balanceOf(bob));
  }

Therefore, the minimum profitable loan should be above 100 DYAD. If at least half of the Notes borrow 100 DYAD and remain underwater without being liquidated, the value of DYAD may depeg.

Tools Used

Foundry

Recommended Mitigation Steps

Implement a check for the minimum mint size when a user attempts to mint DYAD.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L60-L67

Vulnerability details

Impact

Wallets with a large amount of assets (whales) can manipulate the price of Kerosine by depositing/withdrawing from their Notes. Some potential issues that may arise:

• a whale can decrease the price of Kerosine by withdrawing assets from the vault in order to liquidate a user whose CR is close to 1.5
• a whale can deposit assets and increase the price of Kerosine and avoid liquidation, he deposits into different Note (not the one that is close to liquidating) so he can withdraw these assets at any time.

Proof of Concept

As we can see in Vault.kerosine.unbounded.sol

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

the price of Kerosine depends on the TVL and DYAD total supply, the bigger the collateral surplus the bigger the price. This allows users with a large amount of assets to manipulate the price, simply by depositing/withdrawing assets into thr vault.

Check this coded POC in which Bob the Whale removes liquidity from his Note, causing the price to drop which allows him to liquidate Alice.

I used Deploy.V2.s.sol as a setup with some changes: oracles (price 3000e8), dyad and dnft are mocks, run in forked environment.

  function test_Whale() public {
      address alice = address(0xa11ce);
      address bob = address(0xb0b); // <<<------< WHALE
      deal(address(weth), alice, 100e18);
      deal(address(kerosene), alice, 100000e18);
      deal(address(weth), bob, 1000e18);
      
      uint256 aliceId = dNft.mintNft{value: 1 ether}(alice);
      uint256 bobId = dNft.mintNft{value: 1 ether}(bob);
      uint256 bobId2 = dNft.mintNft{value: 1 ether}(bob);

      vm.startPrank(alice);
      weth.approve(address(vaultManager), type(uint256).max);
      kerosene.approve(address(vaultManager), type(uint256).max);
      vaultManager.add(aliceId, address(ethVault));
      vaultManager.add(aliceId, address(unboundedKerosineVault));
      
      vaultManager.deposit(aliceId, address(ethVault), 10e18);
      vaultManager.deposit(aliceId, address(unboundedKerosineVault), 100000e18);
      vaultManager.mintDyad(aliceId, 20000e18, alice);
      vm.stopPrank();
      
      vm.startPrank(bob);
      weth.approve(address(vaultManager), type(uint256).max);
      vaultManager.add(bobId, address(ethVault));
      vaultManager.add(bobId2, address(ethVault));

      vaultManager.deposit(bobId, address(ethVault), 20e18);
      vaultManager.deposit(bobId2, address(ethVault), 100e18);
      vaultManager.mintDyad(bobId, 20000e18, bob);

      vm.roll(1);
      wethOracle.setPrice(2950e8);
      
      // can't liquidate
      vm.expectRevert(IVaultManager.CrTooHigh.selector);
      vaultManager.liquidate(aliceId, bobId);

      vaultManager.withdraw(bobId2, address(ethVault), 100e18, bob);
      // try again
      vaultManager.liquidate(aliceId, bobId);
  }

The same principle can be used if the whale wants to avoid being liquidated. Instead of topping up his soon-to-be-liquidated Note, he can deposit into another Note with no DYAD debt, increasing price of the Kerosine. Then he waits till ETH/USD goes up and withdraw deposited assets. This method differs from depositing into the affected Note because the user doesn't risk his funds.

Tools Used

Foundry

Recommended Mitigation Steps

It's difficult to suggest steps to address this bug without making significant changes to the protocol's architecture, as the Kerosine token is used for calculating the collateral ratio. Perhaps it's possible to decouple collateral ratio calculation logic from Kerosine value.

Assessed type

Other