DYAD - 0xlemon's results

Lines of code

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

Vulnerability details

Impact

In VaultManagerV2 we have non-Kerosine vaults(WETH vault for example) and Kerosine vaults. We have two separate add functions to use them to add the desired vault. vaultLicenser is suppossed to hold the non-Kerosine licensed vaults but if we look at the Deploy.V2.s.sol(which is explicitly said to be in-scope) we can see that vaultLicenser contains both non-Kerosine and Kerosine vaults:

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

This means that users can add a Kerosine vault in the add function meant for adding vault for exogenous collateral.

By doing that the user can successfully bypass the exogenous collateral value check in the VaultManagerV2::withdraw() because the deposited Keroine value will be counted as exogenous collateral even though it isn't. This is quite problematic because we have seen in the past how not having enough exogenous collateral might lead to price disruptions(terrausd - Luna crash).

Proof Of Concept

Lets see the steps how a user can bypass this withdraw check: (Assuming the user has previouslt added other form of collateral)

1. A user calls VaultManagerV2::add and adds a Kerosine vault
2. The user then calls VaultManagerV2::deposit depositing Kerosine tokens
3. When withdrawing we have the following check:

  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
    ...
    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    ...
  }

Now look at the getNonKeroseneValue(id):

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

The loop is through exogenous vaults but since we added a Kerosine vault there and it is licensed by the vaultLicenser, its value will be added to the totalUsdValue.

4. The user successfully withdraws more exogenous collateral than he is supposed to

This also breaks the invariant that the maximum non-Kerosine vaults can be 5 and the Kerosine vaults can be 5.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this in the deploy script when adding vaults to the vaultLicenser add only exogenous vaults, not all of the vaults. Also the vaultLicenser should be used when calculating the TVL for example in Vault.kerosine.unbounded::assetPrice

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Summary

The identified vulnerability in the VaultManagerV2::withdraw function allows for a denial-of-service (DoS) attack on the owner due to the ability of a malicious user to front-run the owner's withdrawal transaction. This attack leverages the restriction that deposit and withdraw operations cannot occur in the same block, causing the owner's withdrawal transaction to revert and effectively denying access to their funds.

Vulnerability Details

The vulnerability stems from the following key aspects of the withdraw function:

• The function checks if the idToBlockOfLastDeposit[id] is equal to the current block number (block.number).

• If this condition is true (indicating that a deposit occurred in the same block), the function reverts with a DepositedInSameBlock error. This check prevents immediate withdrawals following a deposit in the same block. This is generally done to prevent flash loan attacks. However, a malicious user can exploit this behavior:

• By monitoring the blockchain for the owner's withdraw transaction, the malicious user can front-run that transaction and do a deposit of 0 amount within the same block.

• If the malicious user successfully executes their deposit transaction first, it will cause the owner's withdrawal attempt to revert due to the block restriction check. This sequence of events results in a denial-of-service (DoS) attack, where the owner is unable to withdraw their funds as intended.

Proof of Concept

To run add the test and run: forge test --fork-url <your mainnet rpc> --match-test testUserDoSWithdraw -vvv

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import {Test} from "forge-std/Test.sol";
import {Parameters} from "../src/params/Parameters.sol";
import {DeployV2} from "../script/deploy/Deploy.V2.s.sol";
import {Contracts} from "../script/deploy/Deploy.V2.s.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {Vault} from "../src/core/Vault.sol";
import {VaultWstEth} from "../src/core/Vault.wsteth.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault} from "../src/core/Vault.kerosine.bounded.sol";
import {Kerosine} from "../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../src/staking/KerosineDenominator.sol";

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

interface IWETH {
    function deposit() external payable;
    function withdraw(uint) external;
    function approve(address, uint256) external returns (bool);
}

contract VaultManagerV2Test is Test, Parameters {
    Kerosine kerosene;
    Licenser vaultLicenser;
    VaultManagerV2 vaultManager;
    Vault ethVault;
    VaultWstEth wstEth;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;

    IWETH wETH;

    address user = makeAddr("userrrr");
    uint256 userNFTId;

    function setUp() public {
        DeployV2 deployV2 = new DeployV2();
        Contracts memory deployedContracts = deployV2.run();

        kerosene = deployedContracts.kerosene;
        vaultLicenser = deployedContracts.vaultLicenser;
        vaultManager = deployedContracts.vaultManager;
        ethVault = deployedContracts.ethVault;
        wstEth = deployedContracts.wstEth;
        kerosineManager = deployedContracts.kerosineManager;
        unboundedKerosineVault = deployedContracts.unboundedKerosineVault;
        boundedKerosineVault = deployedContracts.boundedKerosineVault;
        kerosineDenominator = deployedContracts.kerosineDenominator;

        wETH = IWETH(MAINNET_WETH);

        vm.deal(user, 1000 ether);
        vm.prank(user);
        wETH.deposit{value: 100 ether}();

        vm.prank(MAINNET_OWNER);
        kerosene.transfer(user, 10_000 ether);

        DNft dNFT = DNft(MAINNET_DNFT);

        vm.prank(user);
        userNFTId = dNFT.mintNft{value: 100 ether}(user);
    }

    function testUserDoSWithdraw() public {
        address randomUser = makeAddr("randomUser");

        vm.startPrank(user);
        vaultManager.add(userNFTId, address(ethVault));
        wETH.approve(address(vaultManager), 10 ether);
        vaultManager.deposit(userNFTId, address(ethVault), 10 ether);
        vm.roll(block.number + 1);
        vm.stopPrank();
        
        // A malicious user front runs the owner's withdraw
        vm.prank(randomUser);
        vaultManager.deposit(userNFTId, address(ethVault), 0);
        
        // The owner tries to withdraw but cannot in the same block because he is fron ran
        vm.prank(user);
        vaultManager.withdraw(
            userNFTId,
            address(unboundedKerosineVault),
            10 ether,
            user
        );
        
    }
}

We can see how by successfully front-running the owner's withdraw transaction the user DoSes the owner.

Impact

This attack disrupts the intended functionality of the withdrawal process and denies the owner access to their assets.

Tools Used

Manual Review

Recommended Mitigation Steps

One way to mitigate this and still leave the flash loan protection is to allow only the owner of the NFT to deposit into his vaults. This way a random user cannot perform this attack.

Assessed type

DoS

Lines of code

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

Vulnerability details

Summary

The VaultManagerV2::withdraw function in the contract is designed to facilitate the withdrawal of assets from a specified vault to a designated recipient. However, the function erroneously attempts to calculate a value using the asset price obtained from the vault's oracle, which is not available for Kerosine vaults. This oversight prevents successful withdrawals of Kerosine tokens and leaves them trapped within the vault.

Vulnerabilities Details

The specific vulnerability arises from the following aspects of the code:

• The line _vault.assetPrice() attempts to retrieve the current asset price from the vault's associated oracle.

• Kerosine vaults do not have an oracle associated with them, as can be seen in Vault.kerosine.

• Accessing _vault.oracle() for a Kerosine vault will result in a failure (revert) due to the absence of this property. As a consequence:

• The calculation of value in the function will fail due to the inability to retrieve the asset price from the nonexistent oracle.

• The failure prevents users from withdrawing Kerosine tokens from the vault, effectively locking the tokens within the vault.

Proof of Concept

Add this test in the test folder and run forge test --fork-url <mainnet rpc url> --match-test testCannotWithdrawKerosene -vvv

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import {Test} from "forge-std/Test.sol";
import {Parameters} from "../src/params/Parameters.sol";
import {DeployV2} from "../script/deploy/Deploy.V2.s.sol";
import {Contracts} from "../script/deploy/Deploy.V2.s.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {Vault} from "../src/core/Vault.sol";
import {VaultWstEth} from "../src/core/Vault.wsteth.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault} from "../src/core/Vault.kerosine.bounded.sol";
import {Kerosine} from "../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../src/staking/KerosineDenominator.sol";

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

interface IWETH {
    function deposit() external payable;
    function withdraw(uint) external;
    function approve(address, uint256) external returns (bool);
}

contract VaultManagerV2Test is Test, Parameters {
    Kerosine kerosene;
    Licenser vaultLicenser;
    VaultManagerV2 vaultManager;
    Vault ethVault;
    VaultWstEth wstEth;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;

    IWETH wETH;

    address user = makeAddr("userrrr");
    uint256 userNFTId;

    function setUp() public {
        DeployV2 deployV2 = new DeployV2();
        Contracts memory deployedContracts = deployV2.run();

        kerosene = deployedContracts.kerosene;
        vaultLicenser = deployedContracts.vaultLicenser;
        vaultManager = deployedContracts.vaultManager;
        ethVault = deployedContracts.ethVault;
        wstEth = deployedContracts.wstEth;
        kerosineManager = deployedContracts.kerosineManager;
        unboundedKerosineVault = deployedContracts.unboundedKerosineVault;
        boundedKerosineVault = deployedContracts.boundedKerosineVault;
        kerosineDenominator = deployedContracts.kerosineDenominator;

        wETH = IWETH(MAINNET_WETH);

        vm.deal(user, 1000 ether);
        vm.prank(user);
        wETH.deposit{value: 100 ether}();

        vm.prank(MAINNET_OWNER);
        kerosene.transfer(user, 10_000 ether);

        DNft dNFT = DNft(MAINNET_DNFT);

        vm.prank(user);
        userNFTId = dNFT.mintNft{value: 100 ether}(user);
    }

    function testCannotWithdrawKerosene() public {
        vm.startPrank(user);
        // Using add() here instead of addKerosene() because of a bug I described in another issue
        vaultManager.add(userNFTId, address(unboundedKerosineVault));
        vaultManager.add(userNFTId, address(ethVault));
        wETH.approve(address(vaultManager), 10 ether);
        vaultManager.deposit(userNFTId, address(ethVault), 10 ether);
        kerosene.approve(address(vaultManager), 1_000 ether);
        vaultManager.deposit(userNFTId, address(unboundedKerosineVault), 1_000 ether);
        vm.roll(block.number + 1);
        //vm.expectRevert();
        // Withdraw() reverts because vault.oracle() is non-existant
        vaultManager.withdraw(
            userNFTId,
            address(unboundedKerosineVault),
            1_000,
            user
        );
        vm.stopPrank();
    }
}

We can clearly see how the withdraw reverts even when we have enough assets to withdraw.

Impact

The impact of this vulnerability is severe for users attempting to withdraw Kerosine tokens from Kerosine vaults. The inability to withdraw Kerosine tokens due to the code revert effectively locks these assets within the vault, rendering them inaccessible to their rightful owners. This is the most important change from the VaultManager to VaultManagerV2 and it cannot be used properly.

Tools Used

Manual Review

Recommended Mitigation Steps

For the VaultManagerV2::withdraw instead of calling the oracle's decimals you can check if that vault has been licensed in the keroseneManager and if that is the case then divide the value by 1e8 (instead of the oracle call) and if not then leave the calculations as they were.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

The identified vulnerability in the VaultManagerV2::withdraw function is related to potential arithmetic underflow during collateral value calculations, leading to unexpected behavior and potential reverts. Specifically, if the collateral value of the NFT (id) is insufficient compared to the value of Kerosine tokens (amount) to be withdrawn, the calculation can result in a negative value leading to an underflow, triggering a revert due to insufficient collateralization. This issue limits the flexibility and usability of the withdrawal process, particularly in scenarios where only Kerosine tokens are deposited or when exogenous collateral value is insufficient compared to Kerosine token value.

Vulnerability Details

The problem is that a user cannot withdraw his Kerosine tokens if the value he wants to withdraw is > his exogenous collateral value. For example a user can only deposit into a Kerosine vault and he will not be able to get his tokens back because of this check in the withdraw 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();
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

This leaves the Kerosine tokens stuck in the vault manager.

Another example is when a user gets liquidated most of his exogenous collateral is taken from him and when he wants to withdraw his Kerosine tokens, he most probably won't be able to because of the underflow that will happen when checking getNonKeroseneValue(id) - value.

Impact

Leaves Kerosine tokens stuck in the contract and the user is denied of his funds.

Tools Used

Manual Review

Recommended Mitigation Steps

I would recommend to construct a different withdraw function for the Kerosine vaults that doesn't include this check. Another solution is to check if the keroseneManager contains the inputed vault and if that is the case then remove the check and leave it as it is if it is another vault.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

Liquidators don't have incentive to liquidate very small positions where the collateral they get is worth less than the dyad value payed + gas used.

Vulnerability Details

Liquidators liquidate users for the profit they can make. If there is no profit to be made than there will be no one to call the liquidate function. For example an account has 4$ worth of collateral and has 4 DYAD minted. This user is undercollateralized and must be liquidated in order to ensure that the protocol remains overcollateralized. Because the value of the account is so low, after gas costs, liquidators will not make a profit liquidating this user. In the end these low value accounts will never get liquidating, leaving the protocol with bad debt and can even cause the protocol to be undercollateralized with enough small value accounts being underwater.

Impact

This can totaly disrupt the protocol if enough small positions are opened. Therefore if there isn't enough collateral to account for the minted DYAD tokens, the price of the DYAD token will depeg and lose its value of 1$.

Tools Used

Manual Review

Recommended Mitigation Steps

A potential fix could be to only allow users to mint DYAD tokens if their collateral value is past a certain threshold.

Assessed type

Oracle

Lines of code

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/src/core/VaultManagerV2.sol#L88

Vulnerability details

Impact

In the DeployV2.s.sol(which is explicitly said to be in scope) the wrong vaults are added to the kerosineManager. Instead of Kerosine vaults, the non-Kerosine ones are added. This is a huge problem because the user cannot use the main functionallity of the VaultManagerV2(the user cannot add Kerosine vaults).

Proof of Concept

Looking at the deploy script we can see that the ethVault and wstEth vaults are added to the kerosineManager instead of the bounded and unbounded Kerosine vaults.

    KerosineManager kerosineManager = new KerosineManager();

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

Looking at the VaultManagerV2::addKerosene function we can see that it checks if the vault that the user wants to add is licensed by the kerosineManager however we saw above that only the wETH and wstETH vaults were added, this means that if the user calls this function with the bounded Kerosine vault the call will revert.

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

Add this test and run forge test --fork-url <your mainnet rpc> --match-test testCannotAddKerosineVault -vvv

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import {Test} from "forge-std/Test.sol";
import {Parameters} from "../src/params/Parameters.sol";
import {DeployV2} from "../script/deploy/Deploy.V2.s.sol";
import {Contracts} from "../script/deploy/Deploy.V2.s.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {Vault} from "../src/core/Vault.sol";
import {VaultWstEth} from "../src/core/Vault.wsteth.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault} from "../src/core/Vault.kerosine.bounded.sol";
import {Kerosine} from "../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../src/staking/KerosineDenominator.sol";

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

interface IWETH {
    function deposit() external payable;
    function withdraw(uint) external;
    function approve(address, uint256) external returns (bool);
}

contract VaultManagerV2Test is Test, Parameters {
    Kerosine kerosene;
    Licenser vaultLicenser;
    VaultManagerV2 vaultManager;
    Vault ethVault;
    VaultWstEth wstEth;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;

    IWETH wETH;

    address user = makeAddr("userrrr");
    uint256 userNFTId;

    function setUp() public {
        DeployV2 deployV2 = new DeployV2();
        Contracts memory deployedContracts = deployV2.run();

        kerosene = deployedContracts.kerosene;
        vaultLicenser = deployedContracts.vaultLicenser;
        vaultManager = deployedContracts.vaultManager;
        ethVault = deployedContracts.ethVault;
        wstEth = deployedContracts.wstEth;
        kerosineManager = deployedContracts.kerosineManager;
        unboundedKerosineVault = deployedContracts.unboundedKerosineVault;
        boundedKerosineVault = deployedContracts.boundedKerosineVault;
        kerosineDenominator = deployedContracts.kerosineDenominator;

        wETH = IWETH(MAINNET_WETH);

        vm.deal(user, 1000 ether);
        vm.prank(user);
        wETH.deposit{value: 100 ether}();

        vm.prank(MAINNET_OWNER);
        kerosene.transfer(user, 10_000 ether);

        DNft dNFT = DNft(MAINNET_DNFT);

        vm.prank(user);
        userNFTId = dNFT.mintNft{value: 100 ether}(user);
    }

    function testCannotAddKerosineVault() public {
        vm.startPrank(user);
        // Using add() here instead of addKerosene() because of a bug I described in another issue
        vaultManager.addKerosene(userNFTId, address(unboundedKerosineVault));
        kerosene.approve(address(vaultManager), 1_000 ether);
        vaultManager.deposit(userNFTId, address(unboundedKerosineVault), 1_000 ether);
        vm.stopPrank();
    }
}

We can see how the most important functionallity is blocked. The addKerosene call reverts. This test is supposed to revert.

Tools Used

Manual Review

Recommended Mitigation Steps

To resolve this issue, in the deploy script instead of adding the non-Kerosine vaults add the Kerosine ones in the kerosineManager

    KerosineManager kerosineManager = new KerosineManager();

-    kerosineManager.add(address(ethVault));
-    kerosineManager.add(address(wstEth));
+    kerosineManager.add(address(unboundedKerosineVault));
+    //kerosineManager.add(address(boundedKerosineVault));

This of course should happen after the Kerosine vaults are deployed. Now we need to change how the asset price is calculated since it should use only the exogenous collateral vaults. The best way is to calculate the TVL using vaultLicenser and in that Licenser we should include only non-Kerosine vaults

Assessed type

Error