DYAD - Egis_Security's results

Lines of code

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

Vulnerability details

Impact

A vault can be licensed (added) by both the Licenser and KeroseneManager, DeployV2 confirms this.

If the same vault is licensed by both licensers, then one vault can be added from both add and addKerosene.

Because the same vault is in both vaults and vaultsKerosene, the value of id2assets will be calculated twice for collatRatio, doubling the collatRatio of an id. This will make positions very hard to liquidate, as the ratio is effectively doubled.

The PoC showcases the issue well.

Note

In DeployV2.sol the vaultLicenser and keroseneManager add both stEthVault and wstEth here and here

Proof of Concept

For the following test, we are using project's current configuration with a few changes, so we can use VaultManagerV2

Inisde test/BaseTest.sol make the following changes:

+ import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
+ import {KerosineManager} from "../src/core/KerosineManager.sol";

contract BaseTest is Test, Parameters {
  DNft         dNft;
  Licenser     vaultManagerLicenser;
  Licenser     vaultLicenser;
  Dyad         dyad;
  VaultManagerV2 vaultManager;
  Payments     payments;

  // weth
  Vault        wethVault;
  ERC20Mock    weth;
  OracleMock   wethOracle;

  // dai
  Vault        daiVault;
  ERC20Mock    dai;
  OracleMock   daiOracle;

  function setUp() public {
    dNft       = new DNft();
    weth       = new ERC20Mock("WETH-TEST", "WETHT");
    wethOracle = new OracleMock(1000e8);

    Contracts memory contracts = new DeployBase().deploy(
      msg.sender,
      address(dNft),
      address(weth),
      address(wethOracle), 
      GOERLI_FEE,
      GOERLI_FEE_RECIPIENT
    );

+    VaultManagerV2 vaultManagerv2 = new VaultManagerV2(dNft, contracts.dyad, contracts.vaultLicenser);
+    KerosineManager kerosineManager = new KerosineManager();
+    vaultManagerv2.setKeroseneManager(kerosineManager);

    vaultManagerLicenser = contracts.vaultManagerLicenser;
    vaultLicenser        = contracts.vaultLicenser;
    dyad                 = contracts.dyad;
+    vaultManager         = vaultManagerv2;
    wethVault            = contracts.vault;
    payments             = contracts.payments;

   ...

    // add the DAI vault
    vm.prank(vaultLicenser.owner());
    vaultLicenser.add(address(daiVault));
+    kerosineManager.add(address(daiVault));
+    vm.prank(vaultLicenser.owner());
+    vaultManagerLicenser.add(address(vaultManagerv2));
  }

Inside test/VaultManager.t.sol add the following test and run it using forge test --mt "test_collatRatioManupulationWhenSameVaultIsAddedForKerosineAndNormal" -vv

 function test_collatRatioManupulationWhenSameVaultIsAddedForKerosineAndNormal() public { 
    uint id = mintDNft();
    uint cr = vaultManager.collatRatio(id);
    assertEq(cr, type(uint).max);

    deposit(dai, id, address(daiVault), 1e22);
    vaultManager.mintDyad(id, 1e15, address(this));
    cr = vaultManager.collatRatio(id);

    // We only call `addKerosene` with the same address and our CR is doubled
    vaultManager.addKerosene(id, address(daiVault));
    assertEq(cr * 2, vaultManager.collatRatio(id));
  }

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

License only non-kerosene vaults from the Licenser and kerosene vaults from KeroseneManager. The alternative is to not allow the same vault to be in both vaults and vaultsKerosene

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

Impact

When calling liquidate, the liquidator is forced to burn all the minted Dyad tokens that are attached to a DNft id.

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

If a whale has a lot of Dyad tokens and then becomes liquidatable, it may be impossible to liquidate him. In the edge case where 1 DNft id has > 50% of the Dyad total supply, it's impossible to liquidate him, as there isn't enough Dyad tokens to cover his debt, which can cause a massive amount of bad debt, as the whale has no incentive of repaying his debt as he know he can't be liquidated.

Tools Used

Manual Review

Recommended Mitigation Steps

Not sure how this can be fixed, limiting the mintable dyad based on an id won't fix the edge case. One way is allowing for partial liquidations, as this way users can chip away at the position, instead of forcing liquidators to liquidate the entire position at once.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/Vault.kerosine.unbounded.sol#L21-L28

Vulnerability details

Impact

When withdraw/redeemDyad is called, the code uses _vault.oracle().decimals() in order to have the correct decimal precision of assets/value.

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

 function redeemDyad(
    uint    id,
    address vault,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
    returns (uint) { 
      dyad.burn(id, msg.sender, amount);
      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;
  }

The problem is that the Vault.kerosene.unbounded.sol (which can be withdrawn from, as shown in the pre-audit video and by the docs) does not have an oracle state variable as it relies getting it's price based on the assets that were licensed by the kerosene manager, so if someone attempts to call withdraw/redeemDyad with vault = unbounded kerosene vault then the tx will simply revert.

This will cause all Kerosene tokens that were deposited to the vault to be stuck, effectively the unbounded vault will act like a bounded vault, but it won't 2x it's usd value.

Tools Used

Manual review

Recommended Mitigation Steps

The two functions have to be reworded so they work with Kerosene vaults or create 2 new functions in the vault manager to handle withdraw/redeem from kerosene vaults.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The protocol has two types of vaults, non-kerosene vaults, which allows users to mint Dyad against their usd value, and kerosene vaults, which only affect the collatRatio and are not accounted for when a user is attempting to mint Dyad.

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

You'll notice this check:

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

This invariant check if the new usd value of non-kerosene vault assets is less than the dyadMinted by the id.

This assumes that withdraw is always called with a non-kerosene vault, but that isn't the case.

If we look at the deploy scripts we see that weth and wstETH are kerosene vaults.

When withdrawing from a kerosene vault, withdraw will always assume that we are withdrawing from a non-kerosene vault and always checks the following, even though kerosene vaults only affect collatRatio and are not taken into account when attempting to mint Dyad tokens.

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

Because of this, users might not be able to withdraw from a kerosene vault, because the protocol always assumes they are withdrawing from a non-kerosene vault.

Proof of Concept

Example:

1. Alice has 2 vaults, 1 non-kerosene vault and 1 kerosene vault.
2. Alice deposits into both vaults and has a collatRatio = 200% to make sure she is safe.
3. Using her non-kerosene vault, she mints dyad, up to the max that she is allowed to.
4. After some time, Alice decides she wants to withdraw some assets from her kerosene vault, because she needs the assets and knows that she has wiggle room in her collatRatio, so she decides to withdraw assets that are going to leave her collatRatio = 170%.
5. She calls withdraw and the tx reverts, as the protocol assumes that she is withdrawing from a non-kerosene vault and subtracts value, which is in kerosene vault terms, from her non-kerosene value, which is incorrect as kerosene vaults and Dyad aren't linked in any way.

Tools Used

Manual Review

Recommended Mitigation Steps

Rework withdraw to look like so:

    vault.withdraw(id, to, amount);
    if (getNonKeroseneValue(id) < dyadMinted) revert NotEnoughExoCollat();

The check is technically the same, but it will only affect users when they are withdrawing from a non-kerosene vault, as that's when getNonKeroseneValue will be lowered.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

• For token such as wsthETH, which is the main token used in the system, deviation is 1% and heartbeat is 1 hour. This may not seem harmful, but in specific edge cases, system may reach state, where dyad.totalSupply() > tvl, which will lead to reverts in calculating kersoine price, which is calculation done in almost all operations (deposit, withdraw, liquidate) Chain of functions for liquidate:

1. https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L213
2. https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L238
3. https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L247
4. https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L262
5. https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.sol#L66
6. Panic revert here -> https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L65

• The system is supposed to use different LSD tokens as collateral and obtain their value using Chainlink Aggregator V3 interface. This could lead to problem with current hardcoded value for stale prices, which is 90 minutes. Some feeds for tokens like rETH have a deviation of 2% and a heartbeat of 24 hours, which may have major impact on position liquidations.

Proof of Concept

Imagine the following scenario:

• wsthEth is valued at $4001 and we have 10 users, each has deposited 1 wsthETH and X kerosine tokens (X is valued at $2000)
• All 10 users mint max possible dyad for their funds
  • Mint  $4000 dyad, because $4001 + $2000 kerosine >= 150%
  • Also $4001 (nonKerosineValue) > $4000
  • As a result TVL(tvl is calculated for only non-kerosine vaults) of the system is 10*4001 = $40 010 and dyad.totalSupply() = $40 000
• wstETH slowly decrease in value over 1 hour and becomes $3965 (0.09% of 4001)
• Currently depositors position are not liquidatable (when they should be), but this is not the major problem
• After 1 hour, chainlink price is updated to $3965 and currently TVL = 10 * 3965 = \$39 650, while dyad.totalSupply() = \$40 000, which is breaking the invariant tvl > dyad.totalSupply()
• The biggest problem is that in this state, those unhealthy are unable to be liquidated, because the following line will panic revert with underflow error (39 650 - 40 000 = error):

uint numerator   = tvl - dyad.totalSupply();

• The problem is that searchers are not able prevent the situation, because this is step-wise jump in the price, which happens instantly

Tools Used

Manual Review

Recommended Mitigation Steps

• Use an oracle, which will fetch the price in real time
• Enforce non-kerosine TVL to be at least 110%

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

Liquidators are incentivized to liquidate other users, because of profits they will make. If there is no profit/incentive, then liquidators have no reason to liquidate other users.

Because the protocol is deployed on Ethereum where gas costs are high, there is a case where the gas costs to liquidate a positions is higher than the profit that the liquidator will make. Thus liquidators will not liquidate such positions, which will leave undercollateralized positions which can potentially turn into bad debt.

If many such positions accumulate in the protocol, it can lead to a lot of bad debt that no one wants to liquidate, giving malicious users incentive to create such positions, as they know they won't be liquidated ever and they can just run away with the minted Dyad.

Similar issue

Tools Used

Manual Review

Recommended Mitigation Steps

Add a minimum amount of collateral that an id needs to have in order to mint dyad.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Anyone can deposit, but only the owner can withdraw/redeemDyad/mintDyad, effectively anyone that isn't an owner and calls deposit, will lose funds.

A malicious way to use the lack of access control is that anyone can DoS remove and removeKerosene as both functions rely that id2assets of the vault are > 0. Since anyone can call deposit, a malicious user can front-run a remove/removeKerosene, depositing dust amounts of vault.asset() using the id of the user that he wants to DoS.

Proof of Concept

Alice has a DNft with id = 1.

1. Alice wants to remove a vault that she no longer uses.
2. Alice calls remove(1, emptyVault).
3. Bob front runs Alice's tx and calls deposit(1, emtpyVault, 1).
4. deposit only checks if the id is valid (not owner by address(0))

modifier isValidDNft(uint id) {
    if (dNft.ownerOf(id) == address(0))   revert InvalidDNft(); _;
  }

The function will increase id2assets[1]. 5. Alice's tx executes and reverts on the following line:

if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();

Note

The natspec inside IVaultManager states that only the DNft owner can call deposit

Tools Used

Manual Review

Recommended Mitigation Steps

Replace isValidDNft with isDNftOwner.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

remove is used to remove a vault that is attached to a DNft id. The function checks if the id has any attached assets to the vault.

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

This can easily be exploited by anyone, by just calling deposit with 1 wei, as that will increase id2asset in the Vault.

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

Proof of Concept

Paste the following inside VaultManager.t.sol and run forge test --mt test_withdrawCanBeDoSed -vv

function test_withdrawCanBeDoSed() public { 
    uint id = mintDNft();
    
    deposit(dai, id, address(daiVault), 1e22);

    vm.expectRevert();
    vaultManager.withdraw(id, address(daiVault), 1, address(this));
  }

Tools Used

Manual review Foundry

Recommended Mitigation Steps

Do not allow deposit to be called by anyone

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

In order to call remove or removeKerosene, the vault has to have id2assets = 0, basically the id must not have any assets in the vault.

Note that this attack doesn't rely on simply calling deposit with dust amounts for an id, this attack can still be pulled off after that issue is fixed.

There are 2 ways for id2assets to increase, through deposit and through move.

move is used exclusively in liquidate. It is possible to weaponize liquidate in such a way as to DoS a removal of an unbounded kerosene vault.

The crux of the issue is that the price of Kerosene is derived from the assets that are licensed by the Kerosene manager.

Proof of Concept

Prerequisites:

1. Alice has 2 DNft's (id = 1 and id = 2)
2. Alice uses id = 1 for her real assets, she deposits in that vault, but doesn't mint anything.
3. For id = 2 she has a kerosene asset vault with no assets and no dyad minted. Bob also uses this vault.
4. The vault that is attached to id = 1 is licensed by the KeroseneManager so the price and amounts of the assets in the vault determine the price of Kerosene.

Because of another issue, it's currently impossible to withdraw assets from a kerosene asset vault due to a missing state variable, but this attack will work regardless.

1. Bob uses an unbounded kerosene vault with id2assets = 0.
2. He wants to call remove so he can remove the vault and then add another one in it's place, as Bob currently has MAX_VAULTS.
3. Bob calls remove.
4. Alice front-runs Bob's call and does the following.
5. Deposits dust amounts into the kerosene asset vault using id = 2 and then mintDyad, up to the collateralization limit.
6. She then mintDyad using the non-kerosene vault using id = 1 so she pushes the price of kerosene down, in order to make id = 2 liquidatable.
7. Alice then calls liquidate(2, Bob's id).
8. liquidate will move the dust amounts of collateral from her vault to Bob's vault.
9. Bob's remove is called and reverts, as his id2assets > 0/

Tools Used

Manual Review

Recommended Mitigation Steps

Change the modifiers that liquidate uses to these:

 function liquidate(
    uint id,
    uint to
  ) 
    external 
      isValidDNft(id)
->    isDNftOwner(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);
          emit Test(cappedCr, liquidationEquityShare, liquidationAssetShare, collateral);
          vault.move(id, to, collateral);
      }
      emit Liquidate(id, msg.sender, to);
  }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Currently, the only restriction to call burnDyad is for the id to be valid (owner != address(0)).

This means that users can burn their Dyad tokens and specify whatever id they wish so that it subtracts from their debt.

This can easily be weaponized by a malicious user, who can front-run a full burnDyad with a very small byrnDyad of 1 wei, which cause an underflow in Dyad.sol#burn()

function burn(
      uint    id, 
      address from,
      uint    amount
  ) external 
      licensedVaultManager 
    {
      _burn(from, amount);
      mintedDyad[msg.sender][id] -= amount;
  }

Proof of Concept

Example:

1. Alice has 100e18 dyad tokens attached to id = 1.
2. She wants to exit her position and withdraw all of her collateral, but first she needs to burn all of her Dyad tokens.
3. Alice calls burnDyad(1, 100e18).
4. Bob front-runs Alice and calls burnDyad(1, 1).
5. Bob tx goes first and mintedDyad[vaultManager][1] = 100e18 - 1.
6. Alice's tx executes and attempts mintedDyad[vaultManager][1] = 100e18 - 1 - 100e18, which panic reverts with an underflow.
7. Bob can continue to DoS Alice when she attempts to burn all her Dyad tokens.

In the worst case scenario this can affect liquidations, as a user that is about to get liquidated may attempt to burn all his Dyad tokens so he doesn't get liquidated, but a malicious user can DoS him enough times so that the liquidation goes through.

Tools Used

Manual Review

Recommended Mitigation Steps

Add the following code inside burnDyad.

if (amount > dyad.mintedDyad(address(this), id)) amount = dyad.mintedDyad(address(this), id);

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L93-L95

Vulnerability details

Impact

If we take a look into DeployV2 script, we notice that kerosine vault is added only to vaultLicenser, which is fine, because if we add it to kerosineManager, another problem arrises, which we will point out later. Now lets assume kerosine vaults are added to vaultLicenser (which is the case, looking into in-scope deploy script). In VaultManagerV2 there are two mappings to collection for vaults (normal and kerosine vaults) and corresponding functions to add such instances for given nftId. We can notice that addKerosine function check whether provided vault address is licensed by the kerosineManager, which we saw is not done in the deploy script and now we will mention why putting such vault into kerosineManager would lead to even major problems:

1. Kersoine vaults added to kerosineManager breaks kerosineVault.aassetPrice() If we look in the function of how UnboundedKerosineVault is calculating price of a single kerosine token, we notice that the vault is iterating over all entities inside kerosineManager and is calling vault.assetPrice() for each of those. So lets suppose we have inserter UnboundedKerosineVault or BoundedKerosineVault inside kerosineManager. This leads to infinite recurrsion on the following line when we reach this kerosine vault, because we reenter the function. This is guaranteed OOG revert on each call of assetPrice(called inside vault.getUsdValue). Impact is DOS of the system, as it would revert on almost all functions inside VaultManagerV2, because they are querying prices from kerosineVaults

2. Kersoine vaults added to vaultLicenser breaks invariant tvl > dyad.totalSupply(), which may also lead to unexpected reverts in kerosineVault.aassetPrice() Lets examine the other possible option. Having kerosineVaults only inside vaultLicenser, so there is no recurrsion inside KersoineVault.assetPrice(). We will add kerosine vaults using add function and they will go inside vaults mapping variable for nft. Now this is a problem, because kerosine vault are counted as non-kerosine vaults. This is a workaround for the following check:

 if (getNonKeroseneValue(id) < newDyadMinted)  revert NotEnoughExoCollat();

which means that user is able to mint dyad valued more than his non-kerosine collateral (If his kerosine collateral meets the collateral ratio requirements) This may break the system invariant, which is tvl(in non-kerosine tokens) > mintedDyad, which may further lead to reverts in calculating bounded/unboundedKersoine.assetPrice here

Proof of Concept

Lets examine the following scenario: For simplicity of the calculations we would assume that 1 kersoine = $1:

• A user add kersoine vault with add function -> it is successfully added to vaults
• The same user calls mintDyad with a value of 660 (his collateral of $1000 kerosine > 150%, so it's healthy collateralized)
• His transaction passes, because getNonKeroseneValue is returning 1000 (his kersoine collateral)
• Now if we assume he is first depositor and dyad.totalSupply after this operation is 660, lets see what will happen inside UnboundedKersoineVault.assetPrice()

  function assetPrice() 
    public 
    view 
    override
    returns (uint) {
      uint tvl;
      address[] memory vaults = kerosineManager.getVaults();
      uint numberOfVaults = vaults.length;
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[i]);
        tvl += vault.asset().balanceOf(address(vault)) 
                * vault.assetPrice() * 1e18
                / (10**vault.asset().decimals()) 
                / (10**vault.oracle().decimals());
      }
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }

tvl would be 0, because user collateral is a kersoineVault, which is licenzed only by vaultLicenzer and kerosineManager vaults still don't hold any value. On the other hand dyad.totalSupply() is 660, because user kersoine collateral allowed user to mint it. As a result uint numerator = tvl - dyad.totalSupply() = 0 - 660 = panic revert.

As we can see protocol haven't taught about where should kersoine manager vaults be licensed

Tools Used

Manual Review

Recommended Mitigation Steps

• Inside vaultLicenser put only non-kerosine vaults and make UnboundedKerosineVault iterate over vaults inside vaultLicenser, instead of kerosineManager.
• Inside kerosineManager put only kersoine vaults

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Protocol's innovative mechanism for dynamically calculating CR using Kerosine ERC20 may open a door for an entirely new liquidation strategy, where liquidators with big capital can create honeypots and ensure they would be liquidators because they can combine withdrawing big part of the TVL(inflating the price of kerosine) and calling liquidate on all victims.

Let's give an example with whale, who is holding 20% of the TVL, but has never minted dyad, so his funds are staying as surplus and is increasing the price of single kerosine token. Now all users which deposit kerosine and mint close dyad close to the liquidation threshold can easily and guaranteed to be liquidated by the whale who would mint dyad for all his collateral deposited, and call liquidate in the same transaction. This is possible because of the formula for calculating kerosine price in the protocol:

    uint tvl;
      address[] memory vaults = kerosineManager.getVaults();
      uint numberOfVaults = vaults.length;
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[i]);
        tvl += vault.asset().balanceOf(address(vault)) 
                * vault.assetPrice() * 1e18 
                / (10**vault.asset().decimals()) 
                / (10**vault.oracle().decimals()); 
      }
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator(); // Kerosine totalSupply
      return numerator * 1e8 / denominator;

After increasing dyad.totalSupply() with 20% for the same tvl, we see that the price would be lower than before.

While this is expected behavior in some terms, it is not fair for searchers, which role is to race for liquidating a user, when his position become unhealthy. By the following concern, there would arise a problem, which is single entity responsible for liquidations and so less searchers in the system.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Obtain kerosine price from previous block

Assessed type

Context