DYAD - alix40's results

Lines of code

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

Vulnerability details

Impact

The protocol allows every user to deposit funds in any dnft (no restrictions for deposits). And as a new Feature, the protocol also implemented a mechanism that is aimed at blocking flashloan attacks and the mechanism will simply revert if a withdrawal happens if there was a deposit to the same DNft in the same Block. Those 2 Facts could be abused by attackers to block other users from withdrawing funds from the protocol: This is done by simply frontruning the withdrawal and depositing 1 wei in a vault on behalf of the user to the user DNft, that he wants to withdraw funds from and the withdraw transaction will revert because of the flashloan guard.

This Attack Vectors is highly desirable for Attackers, as they could benefit from users not being able to withdraw their funds. (For example, to pump up the price of Kerosene)

Proof Of Concept

The deposit() function in the VaultManagerV2 contract is the method that is used to deposit funds into the vaults. As we can see from the code snippet, only the modifier isValidDNft() is used to check if the DNft is valid and doesn't check if the sender is the DNft owner. https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/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);
  }

We could also see that idToBlockOfLastDeposit[id] is set to the current block number in the deposit() function. With those 2 facts, any attacker could frontrun user withdrawals and deposit 1 wei in any of the user Vaults and the user withdrawal transaction will revert because of the flashloan guard. (see code snippet below) https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L134-L142

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

Recommendation

We Recommend allowing only DNft owners to deposit funds in their vaults to mitigate this attack vector.

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

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The bug breaks a system invariant, as it makes Kerosene Tokens deposited in an unbounded kerosene Vaults also unwithdrawable and the problem also arises from the fact that in the bounded Kerosene Vaults at least the users deposited Kerosene Tokens will have twice the value.

Proof Of Concept

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

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

As it is implemented currently the withdraw() function doesn't differentiate between kerosene Vaults and eth Vaults. Eth vaults have the oracle state variable, while Kerosene Vaults, doesn't require an Oracle and therefore don't need it and doesn't implement it.

This will cause _vault.oracle() call to actually revert for Kerosene Vaults (for bounded and unbounded) which will prohibit users from withdrawing Kerosene

To run the Poc please add this test to the test/fork/v2.t.sol file:

function testWithdrawUnboundedPoC() public{
  address userPoc = DNft(MAINNET_DNFT).ownerOf(1);
  console.log("User PoC: ", userPoc);
  // @audit-info we are using add instead of addKerosene because of the bug in the addKerosene/add functions
  vm.prank(userPoc);
  contracts.vaultManager.add(1,address(contracts.unboundedKerosineVault) );
  Kerosine kerosine = Kerosine(MAINNET_KEROSENE);
  console.log("Kerosine balance: ", kerosine.balanceOf(userPoc));
  console.log("Kerosine balance: ", kerosine.balanceOf(MAINNET_OWNER));
  vm.prank(MAINNET_OWNER);
  // fund the the wallet of the user used for PoC
  kerosine.transfer(userPoc, 50 * 1e18);   
  vm.startPrank(userPoc);
  kerosine.approve(address(contracts.vaultManager), 10 * 1e18);
  contracts.vaultManager.deposit(1, address(contracts.unboundedKerosineVault),10 * 1e18);
  console.log("deposit successfull");
  // @audit-info we need to skip next block to avoid reverting because of flashloan protection
  vm.roll(block.number + 1);
  // @audit-info now withdraw from the deposited kerosene in the vault
  contracts.vaultManager.withdraw(1, address(contracts.unboundedKerosineVault),5 * 1e18, userPoc);

}

Result:

Running 1 test for test/fork/v2.t.sol:V2Test
[FAIL. Reason: EvmError: Revert] testWithdrawUnboundedPoC() (gas: 249942)
Logs:
  User PoC:  0x01df211a9c8A9AE434eD2d34404dd7F48b83645C
  Kerosine balance:  0
  Kerosine balance:  950841953470486444914439000
  deposit successfull

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 4.72s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/fork/v2.t.sol:V2Test
[FAIL. Reason: EvmError: Revert] testWithdrawUnboundedPoC() (gas: 249942)

Encountered a total of 1 failing tests, 0 tests succeeded

Recomendation

the withdraw() function should differentiate between kerosene and non-kerosene vaults. Instead of using oracle, directly use the assetPrice() function for kerosene vaults

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

As stated in the white paper for V6, the withdraw() function checks if the user position after withdrawal must fulfill the following invariant check:

$K_{\text{valuewithdraw}} = \frac{(TVL - C_{\text{withdraw}}) - D_{\text{supply}}}{K_{\text{supply}}}$ But as it is stated in the white paper the withdrawal must fullfill this condition only if the the amount being withdrawn is done for non-kerosene Tokens.

The Problem however arises from the fact, that the implementation doesn't differentiate between Kerosene and non-Kerosene Tokens withdrawals. This bug will lead to users not being able to withdraw their deposited Kerosene Tokens if they have not deposited any exogenous Tokens because the invariant check will cause an underflow that will block users from withdrawing their Kerosene Tokens. (even if the users didn't mint any DYAD)

Proof Of Concept

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

    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

As we can see in the Code Snippet the implementation will always checks for the invariant check even for Kerosene Tokens withdrawals.

In the case that there was no exogenous Tokens deposited, part getNonKeroseneValue(id) - value will evaluate to 0 - withdrawl_amount which because of the implicit underflow check in solidity versions > 8 will cause the withdrawal transaction to revert.

Recomendation

In our Opinion, we recommend that the protocol should simply implement a separate function to withdraw Kerosene Tokens. Trying to only offer a single function may make the `withdraw() function more complex and harder to maintain.

Please Note that the withdraw() function have another bug that will always block withdrawal for Kerosene Tokens from unbounded Tokens. please check our Issue with title: Withdrawing Kerosene tokens from Vault.kerosine.unbounded in VaultManagerV2 will always revert

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

Even though The VaultManagerV2.sol changed how the collateralization ratio is calculated in V2. The protocol still uses the same liquidate() function which only seizes collateral from non-kerosene Vault

After confirming the problem with the sponsor, it appears to be that the protocol forgot to iterate through the kerosine vaults and only let the liquidators seize collateral from the Non-Kerosine Vaults

Impact

To better illustrate, the impact of the missing Collateral sent to liquidator, please consider following example:

A user has amount A of WETH woth 1K USD and and have an amount B of Kerosene worth 0.4K of USD The User has minted 1K DYAD:

• The coll ratio for this dNFT is => (400 + 1000) / 1000 ~= 140% it is under the liquidation threshold of 150%
• According to the current fee structure, the liquidator will recieve: 40% * 20% = 8%
• The incentive is => (8% * 100%) / 100 = 8% => the user will be able to withdraw 108%/140% of non Kerosene Collateral => the liquidator will seize ~77% of the user non kerosene collateral => the liquidator will recieve 77% * amount A of WeTh which is worth 770 USD and take this the user will need to pay 1 K Dyad, which is equivalent to a 330 USD loss for the liquidator

As we can see the The liquidation fee mechanism includes the Kerosene value in the fee calculation, but allows the liquidator to only seize non Kerosene collateral.

This structure may have worked perfectly for the first version of the protocol, where kerosene vaults didn't exist. But now if a user puts a relevant amount of kerosene in his position, The liquidation of the position in most cases will be unprofitable for the liquidator Such a rewarding structure could break the protocol and lead to total insolvancy. For this reason we have set the severity as high

Proof Of Concept

the liquidate function in the VaultManagerV2.sol is the exact same as the one in the VaultManager.sol

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/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);
  }

collatRatio is calculated following the following formula: => value_usd_Kerosene_Collateral + value_usd_Exo_Collateral / total_minted_dyad

And the liquidation incentive is calculated from this collateral ratio, which includes the kerosene Value in the collateral value. We could later see that in the for loop the protocol iterates on the non kerosene vaults, and takes the calculated seize percantage of the non kerosene collateral. And because of this discrepancy the liquidationAssetShare is actually incorrect as the liquidator wouldn't get the (liquidationEquityShare + 1e18) of the kerosene collateral, but only this percentage of the non kerosene collateral. which leads to less incentive than deserved

As I showed in the e.g in the first section, This would lead to the liquidator being way down in a loss.

Recommendation

The protocol needs to iterate through the kerosine vaults in the liquidate() function and move the seized kerozen to Dnft of the liquidator. e.g fix would be like this:

  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);
      }
+      uint numberOfKeroseneVaults = vaultsKerosene[id].length();
+      for (uint i = 0; i < numberOfVaults; i++) {
+          Vault vault      = Vault(vaultsKerosene[id].at(i));
+          uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
+          vault.move(id, to, collateral);
+      }
      emit Liquidate(id, msg.sender, to);
  }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Flash loan attacks are still possible and attackers are still able to perform flash loan attacks to maniputlate Kerosene Price

Proof Of Concept

Performing a flashloan attack is still possible, the attack is not as direct as it was by simply depositing and withdrawing. The attack goes like this:

• Attacker first prepares 2 DNFTs, in DNft2 he will deposits some WETH (some means also small amount like 0,3 weth) and in the DNft1 he will deposit 300 usd worth of Kerosene
• Attackers wait 1 block
• Attackers takes a flashloan, deposits it in MarketManagerV2
• Attackers now goes and mints the max possible amount of dyad from DNft 1: e.g if he deposited 14.7K worth of weth from the flash loan along the 300 usd worth of Kerosene he deposited earlier, he mints 10k dyad so the Collateralization Ratio is max (150%)
• The Attacker goes now on his DNft2 withdraws a small amount of his weth in order to drop the price of Kerosene a little to make collateralization ratio smaller than 150% (equal to 149,99999%)
• The Attacker goes now in the same Transaction and liquidates his own position that he recently forced to make it liquidatable. The attacker will use the dyad he minted to liquidate Position on DNft1 and he will get 73,33% of his deposited weth. He still have 26.66% locked in the contract.

The attacker will simply keeps on repeating the attack untill the remaining value of Weth in the contract is negligble, with each iteration reducing the amount he have in the vault by 73,33%.

E.g calculations of how many iterations required and how much percentage of weth still left in the contract

After only 3 Iteration of the attack the attacker would be able to withdraw 98,1% of his deposited funds without paying any fees other than gas fees. (and the small funds that are left in the contract he can still withdraw in the next block) Following this strategy an attacker is able to bypass the flash loan security guard and perform any price manipulation he wants using flash loans

Recomendation

To mitigate this attack vector, we would recommend tracking dyad minting through mintDyad() and to only allow 1 dyad minting operation per block.

Assessed type

Context

Lines of code

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

Vulnerability details

Description

It might have been less risky to ignore this issue in the prior version of the protocol, As before the addition of Kerosene the minimum Collateral Ratio was 150%. and the liquidation incentive is 20% which is really high and would lead in most cases to positions being liquidated before the collateralization ratio drops under 100%.

With the addition of kerosene which would allow the users to mint dyads worth 1:1 to the value of their exo collateral. The risk of positions going under (debt worth more than collateral) is significantly higher. This is also due to the fact that kerosene value is highly dependent of the value of excess collateral and the number of kerosene tokens being distributed (not locked in the main account). This could lead to a volatile price of kerosene and will lead to actually higher volatility in the market.

E.g: as it is now only ETH is presented as collateral: meaning the price of kerosene corrolate indirectly with the price of ETH. A drop of 10% in ETH could result in a drop of 10% in the price of kerosene too. The less the excess collateral is, the more the value of Kerosene will drop. Please also note that in the case where prices drop. Users tend to rush in withdrawing their Collateral and for example exchanging it for Stable Coins to avoid risk. This will also lead in the price of Kerosene dropping significantly (less deposits == less excess collateral but number of kerosene Tokens supply stays the system) and also increasing the risk of liquidations

Due to the amount of risk assossiated with the addition of Kerosene, having no mechanism or structure in place to handle underwater debts, could lead to the insolvancy of the protocol in times of high volatility

Recomendation

With the addition of Kerosene, the protocol in our opinion must implement a structure to handle underwater debt. The option we would recommend is to implement a stability pool (recomended most) or/with socializing bad debt

Assessed type

Context

Lines of code

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

Vulnerability details

Description

Bounded Kerosene is not attractive for liquidation. First of all bounded kerosene has twice the value of unbounded kerosene. E.g if 1 Kerosene is worth 10 usd in the unbounded vault then 0.5 Kerosen will be worth the same amount in a bounded vault. Twice the valuation is justified for users and Liquidity Providers. The problem however is if we take the prespective of a liquidator. If he liquidates a position that has a significant percentage of the position value in bounded Kerosene Vault. The liquidator will recieve the amount locked in the bounded kerosene vault, which he can't withdraw so the liquidator actually wouldn't be necessary be able to swap the kerosene against stable coins to make a profit for example. He will also recieve half the amount of kerosene if this was an unbounded kerosene Vault. This might not be clear, so we encourage the reader to see the following example to understand the need to rework liquidations for bounded kerosene vaults.

N.B please note that there is a bug in the liquidate() function and Kerosene Collateral are not sent to the liquidator, but as I have confirmed with the sponsor, The protocol intended to send the Kerosene Tokens to the liquidators along side the seized Eth

Proof of Concept

As an example to show the huge disadvantage a liquidator will face when liquidating bounded kerosene vs unbounded kerosen vs exo collateral (like eth)

let's take similar context but with different composition of collateral

Collateral worth 1.4k and minted dyad is worth 1k usd: -> collRatio =140 % which is elligible for liquidation. percentage of coll to seize is (100%+ 40%*20%)/140% => 77.14% of collateral to seize

For simplicity let's say 1 unbounded kerosene Token is worth 100 usd and 1 bounded kerosene Token is worth 200 usd (twice the asset price) in the vaults as collateral ( however the value of Kerosene outside the vaults in exchanges is gonna be near the 100 usd valuation)

The liquidator needs to pay down 1k dyads (1k usd)

Recomendation

Having twice the value for kerosene if locked, might make sense for Protocol user, this however will result in problems for liquidators. We recommend the protocol to implement a solution that addresses the following issues:

• Liquidators of kerosene type collateral, should always be able to withdraw the token (whetether seized from bounded or unbounded Vaults)
• When calculating the amount of tokens to seize (incentive), Kerosene tokens should be valued the same, for unbounded and bounded kerosene Vaults, so it would make economic sense for the liquidator

Assessed type

Context

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

remove() and removeKerosene() could be Dossed by depositing 1 wei of collateral The VaultManagerV2 allows any one to deposit collateral assocciated with a DNft and doesn’t force the sender to be theDNft owner

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); } remove() and removeKerosene() however checks that the removed Vaults from the MarketManagerV2 must have 0 tokens posted as collateral. And in this case it is possible for an attacker to deposit 1 wei to the vault in the name of the dnft which will make vault removal revert

Lines of code

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

Vulnerability details

Impact

The implementation of the adding vaults functions (kerosene and non-kerosene) is not implemented correctly. This is a high severity bug as it will allow users to actually add kerosene-vaults as non-kerosene vaults. Which would make it possible for users to mint dyad fully against Kerosene Tokens (Instead of supplying ETH, to be able to do it). Another bug in the MarketManagerV2 caused by the same false assumption, is that in getKeroseneValue() the function will iterate through all the kerosene vaults assossiated with Dnft but will always return 0 as value.

Proof Of Concept

The KerosineManager is used to deterministaclly determine the price of Kerosene and in order to know the price of the exogenous collateral in the dyad ecosystem. For this the KerosineManager only track the exogenous collateral vaults, and not the kerosene vaults and because of this the KerosineManager.isLicensed() function will always return False for kerosene vaults and will return True for eth and steth vaults.

First Issue:
Proof of concept is straight forward, we will just add the unboundedKerosineVault using add() (which is desired for non kerosene vaults) and we will add wstEth vault using addKerosene() function. Both of those actions should revert, if both functions where implemented correctly To run the proof of Concept, please add the following test to the testsuite in test/fork/v2.t.sol

  function testAddPoC() public {
    address userPoc = DNft(MAINNET_DNFT).ownerOf(1);
    vm.prank(userPoc);
    contracts.vaultManager.add(1,address(contracts.unboundedKerosineVault) );
    vm.prank(userPoc);
    contracts.vaultManager.addKerosene(1,address(contracts.wstEth) );
    address[] memory NonKeroseneVaults = contracts.vaultManager.getVaults(1);
    assertTrue(NonKeroseneVaults.length == 1);
    assertTrue(NonKeroseneVaults[0] == address(contracts.unboundedKerosineVault));
  }

Result:

Running 1 test for test/fork/v2.t.sol:V2Test
[PASS] testAddPoC() (gas: 176259)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.70s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Second Issue:
For the second bug in the getKeroseneValue() function, we can see that the function will iterate over all the kerosene vaults associated with the dnft, and add the value of the vaults to the total. The problem is however that the value of Kerosene Vaults is only added, if the check keroseneManager.isLicensed(vault) returns True, which is never the case.

  function getKeroseneValue(
    uint id
  ) 
    public 
    view
    returns (uint) {
      uint totalUsdValue;
      uint numberOfVaults = vaultsKerosene[id].length(); 
      // NOTE iterate through all the kerosene vaults
      for (uint i = 0; i < numberOfVaults; i++) {
        // NOTE get usd value from kerosene vault
        Vault vault = Vault(vaultsKerosene[id].at(i));
        uint usdValue;
        // @audit-issue Will always return false for kerosene vaults
        if (keroseneManager.isLicensed(address(vault))) {
          
          usdValue = vault.getUsdValue(id);        
        
        }
        totalUsdValue += usdValue;
      }
      return totalUsdValue;
  }

Recomendation

As a Fix we recommend using KerosineManager.isLicensed(vault)==True to check for exogenous collateral, and KerosineManager.isLicensed(vault)==False along with KerosineManager.isLicensed(vault)==True to check for kerosene vaults. To give an example on how to fix the add functions:
Please add the following fixes to how the Kerosen and non Kerosene vaults are added in the add and addKerosene functions in the VaultManagerV2 contract. The fix aims for blocking users from adding a kerosene vault as a normal vault and from adding a normal vault as a kerosene vault.
(KerosineManager.isLicensed(vault) would return True for eth and steth vaults but will deliver False for every other address)

  function add(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
+    if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();

    if (!vaults[id].add(vault))            revert VaultAlreadyAdded();
    emit Added(id, vault);
  }
+  error NotAKeroseneVault();

  function addKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
+    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();

-    if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
+    if (keroseneManager.isLicensed(vault))                 revert NotAKeroseneVault();
    if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
    emit Added(id, vault);

Assessed type

Invalid Validation