DYAD - Al-Qa-qa's results

Lines of code

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

Vulnerability details

Summary

The same vault collateral can be used twice in calculating the CR, leading to doubling the CR ratio to its real value.

Impact

There are two managers for vaults, VaultManager and KeroseneManager.

VaultManager licenses all Vaults supported for collateral either exogenous like wethVault or endogenous like keroseneBoundedVault.

KeroseneManager licenses only exogenous Vaults supported for collateral, which means it will not license the Kerosene Vaults.

Two mappings handle these variables:

VaultManagerV2.sol#L34-L35

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

The problem is that when we calculate the CR, we add the nonKeroseneVaults Collateral with the keroseneVaults collateral.

VaultManagerV2.sol#L230-L248

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

  // @audit Adding Collateral in Kerosene and nonKerosene vaults and divide them by the position DYAD supply
  function getTotalUsdValue(uint id) public view returns (uint) {
      return getNonKeroseneValue(id) + getKeroseneValue(id);
  }

So if the vault is added in both vaults and vaultsKerosene, the value of the collateral will be added twice.

VaultManagerV2.sol#L250-L286

  function getNonKeroseneValue(uint id) public view returns (uint) {
      uint totalUsdValue;
      uint numberOfVaults = vaults[id].length(); // @audit NonKeroseneVaults
      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; // @audit add collateral value if the vault is Licenced
      }
      return totalUsdValue;
  }

  function getKeroseneValue(uint id) public view returns (uint) {
      uint totalUsdValue;
      uint numberOfVaults = vaultsKerosene[id].length(); // @audit KeroseneVaults
      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); // @audit add collateral value if the vault is Licenced
        }
        totalUsdValue += usdValue;
      }
      return totalUsdValue;
  }

The problem here is that the Vaults should be duplicated. All exogenous vaults will be in KeroseneManager, where the KeroseneManager vaults are used to calculate Kersone price.

├─ KerosineManager - "Add/Remove Vaults to the Kerosene Calculation"

And this is what the team does in the deployment script.

In brief, the collateral ratio will get calculated wrongly and this will cause massive issues.

• The user can mint DYAD at a CR ratio smaller than 150% without getting his position liquidated.
• The user can withdraw his collateral and make his position go below 150% without getting his position liquidated.
• Undercollateralized position will be unliquidatable.

This will put the Coin Price in danger, where the collateralization process will totally break.

All these issues happen because collatRatio() function and the main issue is in getTotalUsdValue(). We will tread these two issues

1. collateralRatio() is incorrectly implemented leading to incorrect values.
2. getTotalUsdValue() adds duplicate vault collateral value leading to the incorrect price.

as one issue as the root cause is the same, which lies in getTotalUsdValue() function.

Proof of Concept

I made a PoC that shows how the collateral ratio will be 300%, when it should be 150% according to the collateral value.

I wrote my PoC script in a separate file in the /test folder. you can make a file in the /test folder, give it a name. and put the following code inside.

<details> <summary> Expand Code </summary>

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

import "forge-std/console.sol";
import {VaultManagerTestHelper} from "./VaultManagerHelper.t.sol";
import {IVaultManager} from "../src/interfaces/IVaultManager.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";
import {ERC20Mock} from "./ERC20Mock.sol";

// ----
import {Parameters}             from "../../src/params/Parameters.sol";
import {VaultManagerV2}         from "../../src/core/VaultManagerV2.sol";
import {DNft}                   from "../../src/core/DNft.sol";
import {Dyad}                   from "../../src/core/Dyad.sol";
import {Licenser}               from "../../src/core/Licenser.sol";
import {Vault}                  from "../../src/core/Vault.sol";
import {VaultWstEth}            from "../../src/core/Vault.wsteth.sol";
import {IWETH}                  from "../../src/interfaces/IWETH.sol";
import {IAggregatorV3}          from "../../src/interfaces/IAggregatorV3.sol";
import {KerosineManager}        from "../../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault}   from "../../src/core/Vault.kerosine.bounded.sol";
import {Kerosine}               from "../../src/staking/Kerosine.sol";
import {KerosineDenominator}    from "../../src/staking/KerosineDenominator.sol";

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

struct Contracts {
  Kerosine               kerosene;
  Licenser               vaultLicenser;
  VaultManagerV2         vaultManager;
  Vault                  ethVault;
  VaultWstEth            wstEth;
  KerosineManager        kerosineManager;
  UnboundedKerosineVault unboundedKerosineVault;
  BoundedKerosineVault   boundedKerosineVault;
  KerosineDenominator    kerosineDenominator;
}


contract VaultManagerTest is VaultManagerTestHelper {

  VaultManagerV2 vaultManagerV2;

  function test_duplicate_vaults() external {

    Contracts memory contracts = deployV2ManagerMock();

    vaultManagerV2 = contracts.vaultManager;
    Vault ethVaultV2 = contracts.ethVault;

    // ETH/USD price is 3,000
    wethOracle.setPrice(3000e8);

    address liquidityProvider = makeAddr("LP");

    uint256 id = dNft.mintNft{value: 1 ether}(liquidityProvider);
    
    // Adding 10ETH as collateral and mint 20,000 DYAD (CR = 150%)
    vm.startPrank(liquidityProvider);
    __deposit(weth, liquidityProvider, id, address(ethVaultV2), 10e18);
    vaultManagerV2.mintDyad(id, 20_000e18, liquidityProvider);
  
    console.log("collateralratio: %e", vaultManagerV2.collatRatio(id));
  
    // Adding kerosene Vault 
    // NOTE: any dNFT holder can add a vault if it is licenced
    vaultManagerV2.addKerosene(id, address(ethVaultV2));

    console.log("collateralratio: %e", vaultManagerV2.collatRatio(id));
    vm.stopPrank();

  }


  function __deposit(
    ERC20Mock token,
    address minter,
    uint      id,
    address   vault,
    uint      amount
  ) public {
    vaultManagerV2.add(id, vault);
    token.mint(minter, amount);
    token.approve(address(vaultManagerV2), amount);
    vaultManagerV2.deposit(id, address(vault), amount);
  }

  /*
    We are deploying a Mock version of VaultManagerV2 where:
    - We only add wethVault as Licenced Vault
    - Add wethVault into kerosene vaules
  */
  function deployV2ManagerMock() internal returns (Contracts memory){

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

    vm.prank(vaultManagerLicenser.owner());
    vaultManagerLicenser.add(address(__vaultManagerV2));

    KerosineManager kerosineManager = new KerosineManager();

    Vault ethVaultV2 = new Vault(
      __vaultManagerV2,
      weth,
      IAggregatorV3(address(wethOracle))
    );

    kerosineManager.add(address(ethVaultV2));

    __vaultManagerV2.setKeroseneManager(kerosineManager);

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

    return Contracts(
      Kerosine(MAINNET_KEROSENE),
      vaultLicenser,
      __vaultManagerV2,
      ethVaultV2,
      VaultWstEth(address(0)),
      kerosineManager,
      UnboundedKerosineVault(address(0)),
      BoundedKerosineVault(address(0)),
      KerosineDenominator(address(0))
    );

  }

}

</details>

Then, you can run this command to run it.

forge test --mt test_duplicate_vaults -vv

Tools Used

Manual Review + Foundry

Recommended Mitigation Steps

In the current implementation, nonKerosineVaults should have all vaults supported. But to be sure we can make a Set for the vaults that are existed in Both kerosine and nonKerosine vaults. and use it to get the total USD value.

Here is the new implementation of getTotalUsdValue() function after modification.

  function getTotalUsdValue(
    uint id
  ) 
    public 
    view
    returns (uint) {
      uint totalUsdValue;

      address[] memory uniqueVaults = getUniqueVaults(id);

      for (uint i = 0; i < uniqueVaults.length; i++) {
        Vault vault = Vault(uniqueVaults[i]);
        uint usdValue;
        if (vaultLicenser.isLicensed(address(vault)) || keroseneManager.isLicensed(address(vault))) {
          usdValue = vault.getUsdValue(id);        
        }
        totalUsdValue += usdValue;
      }
      return totalUsdValue;
  }

  function getUniqueVaults(uint id) public view returns (address[] memory) {
    uint numberOfVaults = vaults[id].length() + vaultsKerosene[id].length(); 

    address[] memory uniqueVaults = new address[](numberOfVaults);

    uint uniqueIndex = 0;
    
    // Add all Vaults in the beginning
    for (uint i = 0; i < vaults[id].length(); i++) {
      address vaultAddress = vaults[id].at(i);
      uniqueVaults[uniqueIndex] = vaultAddress;
      uniqueIndex++;
    }
    
    // Add only KeroseneVaults if they are not existed
    for (uint j = 0; j < vaultsKerosene[id].length(); j++) {
      address vaultKeroseneAddress = vaultsKerosene[id].at(j);
      if (!isAddressInArray(uniqueVaults, vaultKeroseneAddress, uniqueIndex)) {
        uniqueVaults[uniqueIndex] = vaultKeroseneAddress;
        uniqueIndex++;
      }
    }

    return uniqueVaults;
  }

  function isAddressInArray(address[] memory arr, address _address, uint length) internal pure returns (bool) {
    for (uint i = 0; i < length; i++) {
      if (arr[i] == _address) {
        return true;
      }
    }
    return false;
  }

This mitigation is not Gas efficient, But I tried only to change the function logic. And it can be optimised by adding some Storage Variables like a mapping for all Vaults.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

For a user to withdraw his collaterals, he calls VaultManagerV2::withdraw.

The function gets the value of the USD relative to the amount of assets the user will withdraw, to check the CR ratio.

But to get the actual value of USD to that amount of collaterals to be withdrawn. it fires Vault.oracle() function, to get oracle decimals.

VaultManagerV2.sol#L146-L149

  function withdraw( ... ) 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 Kerosine vaults do not have `oracle` interface
                  / 10**_vault.asset().decimals();
    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

The problem here is that oracle() function existed only in exogenous collateral vaults, but kerosine vaults (endogenous) do not implement this function.

oracle() refers to the ChainLink Aggregator Contract address which retrieves exchanging price. However, the kerosine price is not determined by an oracle, so vaults do not implement this function.

This will lead to reverting of withdraw() transaction when the user wants to withdraw his collaterals from kerosine vaults.

There are two types of kerosine vaults:

• UnBounded: The user should be able to withdraw collateral, but he will not be able to do this.
• Bounded: the user can not withdraw his collaterals, the function will revert before reaching the withdrawing point (we can tolerate this).

This will make all kerosine tokens locked in UnBounded vault. and users will not be able to withdraw them.

The issue also affects VaultManagerV2::redeemDyad() function.

VaultManagerV2.sol#L195-L198

  function redeemDyad( ... ) external isDNftOwner(id) returns (uint) { 
      ...
      uint asset = amount 
❌️                  * (10**(_vault.oracle().decimals() + _vault.asset().decimals())) 
                    / _vault.assetPrice() 
                    / 1e18;
      withdraw(id, vault, asset, to);
      ...
  }

Since redeem is used to withdraw collaterals too, but by providing the amount of DYAD to be burned. we see that the root cause is the same, and mitigation is the same too, So we will combine these two issues in That report.

Proof of Concept

• Users earn Kerosine tokens from Staking.
• Users deposit some Kerosine into UnBounded Vault through VaultManagerV2::deposit().
• Users want to withdraw some of their kerosine tokens.
• The function will always revert because of the absence of the oracle() function in the UnboundedkerosineVault.
• All users' funds will be locked permanently, and they can't be recovered.

Tools Used

Manual Review + Foundry

Recommended Mitigation Steps

There are two things we can do to mitigate this issue.

1: Implement oracle() interface to kerosine vaults

We can make a simple hack to make oracle() returns address(this), to mitigate the issue with a simple modification.

Vault.kerosine.sol

abstract contract KerosineVault is IVault, Owned(msg.sender) {
  using SafeTransferLib for ERC20;

  IVaultManager   public immutable vaultManager;
  ERC20           public immutable asset;
  KerosineManager public immutable kerosineManager;
+ KerosineVault   public immutable oracle;
+ uint8           public immutable decimals;

  mapping(uint => uint) public id2asset;

  modifier onlyVaultManager() {
    if (msg.sender != address(vaultManager)) revert NotVaultManager();
    _;
  }

  constructor(
    IVaultManager   _vaultManager,
    ERC20           _asset, 
    KerosineManager _kerosineManager 
  ) {
    vaultManager    = _vaultManager;
    asset           = _asset;
    kerosineManager = _kerosineManager;
+   oracle          = KerosineVault(address(this));
+   decimals        = 8;
  }

  ...
}

2: Make another function that supports withdrawing from kerosine vaults in VaultManagerV2

You can make another function to withdraw from the vaults that do not implement oracle() function. But this can introduce some complexities.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The team provided a solution for FlashLoan attacks, by preventing deposit and withdrawing at the same block. However, this check is not done in VaultManagerV2::liquidate() function.

The idea for that protection is to prevent the users from plumbing up and down the kerosine price, where its value is determined by the total DYAD minted.

Vault.kerosine.unbounded.sol#L65

  function assetPrice() public view override returns (uint) {
      ...

❌️    uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }

So if the DYAD total supply increased significantly, kerosine price would go down, and here is how the attack is profitable.

Since users can provide kerosine into their position as endogenous collateral. Decreasing the price of kerosine significantly, will make their position vulnerable to undercollateralization and being liquidatable.

But what about FlashLoan protection? we prevent withdrawing in the same block!

The problem here is that this protection is for the position ID that deposited collaterals, so:

• If I deposited for ID = 1, I will be unable to withdraw at the same block from ID = 1.
• But if I deposited for ID = 1, I will still be able to withdraw at the same block from ID = 2.

Since the liquidation reward is huge ~73% even if the CR is less than 1.5e18 by 1 wei, the receiver dNFT ID only needs to have 27% of collaterals to be able to pay for the flashloan. [ref]

Proof of Concept

So what the attacker can do:

• Check for positions that can go undercollateralized if kerosine price goes down.
• Borrow a FlashLoan (Not a big amount, just to make the targeted position undercollateralized even by 1 wei).
• deposit to an ID he owns and mint DYAD.
• Kerosine prices decreased significantly, and the targeted position became undercollateralized.
• Liquidate the targeted position, and send the funds to another ID he owns (owned by the attacker).
• Withdraw collaterals from the ID that received the collateral (will not revert as this is a different ID).
• User pays for the FlashLoan using the funds he withdrew from the other ID. which can be 73% of the total value borrowed + the remaining amount needed to be paid for the FlashLoan contract.

There are three conditions for that attack to occur:

1. Borrow FlashLoan Collaterals that is exactly enough to liquidate the victim (i.e. make CR exactly equal to 150% or a little more of the DYAD to be minted).
2. The Minted Dyad using this flash loan should make kerosine value decrease in a way that the targeted position goes undercollateralized.
3. The user should have ~27% worth of value of the FlashLoan. As the liquidation reward is ~73% in that case.

So for example, if a position has 20,000 DYAD.

• I should borrow from a flash loan which makes me exactly mint 20,000 DYAD.
• Minting 20,000 DYAD, decreases kerosine price, causing the targeted position to be undercollateralized.
• I should have 27% of the value of the FlashLoan I borrowed as I will only receive 73% of the value I borrowed.

NOTE: the remaining amount (27%) I can pay it to the flash loan after finishing interacting with VaultManagerV2. where I will withdraw the amount 73% and add by 27% to them and pay back the flash loan.

NOTE: since CR < 1.e18% by 1 wei or sometine, The Collateral to USD value will be the same or less by a negligible amount.

These are the edges of the three conditions. If The attacker should mint 30,000 DYAD to undercollateralized that position (20,000 is not enough), he will need to have more than 27% of the FlashLoan he borrowed.

This attack may be inapplicable at the time of launch as kerosine supply is small, which means the positions will not use them as collaterals that much. But as time passes, kerosine will get distributed to the stakers, and they will use them as collateral for their position which increases the likelihood of the attack.

In brief, The likelihood of the attack is LOW as we see it, but it increases over time. The Impact is large, as this will incentivize attackers to manipulate kerosine price, and liquidate positions causing instability for the coin and users' loss of funds (and they get a large reward for that when liquidating positions). I chose MEDIUM severity.

Tools Used

Manual Review

Recommended Mitigation Steps

Prevent taking DYAD from a dNFT ID that is deposited in the same block.

VaultManagerV2::liquidate()

  function liquidate( ... ) ...  {
+     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
      uint cr = collatRatio(id);
      if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
      dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));
    
      ...
  }

Assessed type

Other

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

[L-01] BoundedKerosineVault::setUnboundedKerosineVault is not fired when deploying, leading to DoS when they are added. For BoundedKerosineVault to return the assetPrice, it depends on the UnBoundedkerosineVault asset price, and multiply it by 2.

Vault.kerosine.bounded.sol#L44-L50

function assetPrice() public view override returns (uint) { return unboundedKerosineVault.assetPrice() * 2; } And unboundedKerosineVault should be set by the owner of the contract.

Vault.kerosine.bounded.sol#L23-L30

function setUnboundedKerosineVault( UnboundedKerosineVault _unboundedKerosineVault ) external onlyOwner { unboundedKerosineVault = _unboundedKerosineVault; } In Deploy.V2.s.sol, the vault is not set before transferring the ownership from the Deployer to the MAINNET_OWNER (multi-sig). which makes adding this vault leads to reverting when calling different functions in the VaultManagerV2, and DoS the protocol.

Deploy.V2.s.sol#L78-L91

function run() public returns (Contracts memory) { ...

UnboundedKerosineVault unboundedKerosineVault = new UnboundedKerosineVault( ... );

BoundedKerosineVault boundedKerosineVault     = new BoundedKerosineVault( ... );

KerosineDenominator kerosineDenominator       = new KerosineDenominator( ... );

unboundedKerosineVault.setDenominator(kerosineDenominator);

// @audit we did not set the unBoundedVault in `boundedKerosineVault` before transfering the ownership

unboundedKerosineVault.transferOwnership(MAINNET_OWNER);
boundedKerosineVault.  transferOwnership(MAINNET_OWNER);

} PoC The protocol Deployed into the mainnet. The ownership of the boundedKerosineVault transferred to multi-sig. boundedKerosineVault was added as licensed vaults. The owner of the dNFT owners added that vault. VaultManagerV2::getNonKeroseneValue, VaultManagerV2::getKeroseneValue, and VaultManagerV2::getTotalUsdValue will revert when getting called, as they call getUsdValue which calls assetPrice, which will make a call to the address(0), and it will revert. If the vault is added as a NonKerosene vault getNonKeroseneValue() and getTotalUsdValue() will revert. If the vault is added as Kerosene vault getKeroseneValue() and getTotalUsdValue() will revert If the vault is added to both then all functions will get reverted (if it is a licensed vault as kerosene vault and normal vault). This will make all the protocols in DoS, as these functions are called when minting, withdrawing, and liquidating.

In the Deployment script, all settings and configurations occur to all newly deployed contracts before transferring the ownership to MAINNET_OWNER, So we believe that this is an issue.

Recommendations Set the unBoundedKeroseneVault before transferring the ownership.

Deploy.V2.s.sol L:89

function run() public returns (Contracts memory) { ...

unboundedKerosineVault.setDenominator(kerosineDenominator);

• boundedKerosineVault.setUnboundedKerosineVault(unboundedKerosineVault);

  unboundedKerosineVault.transferOwnership(MAINNET_OWNER); boundedKerosineVault. transferOwnership(MAINNET_OWNER);

  ... }

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

[L-02] No minimum threshold for positions makes so small positions unprofitable to get liquidated the owners of the dNFT can mint DYAD by providing collateral equal to 150%. and if his position gets undercollateralized. people can liquidate him and get a reward for that.

But there is no minimum minting limit for the position. So if the NFT owner made a position with a small amount, and after time this position gets undercollateralized. it can be unprofitable to liquidate that position as the return may be too little compared to the gas that the liquidator will pay for the calling.

POC The owner deposited an amount of ETH. The user minted some small DYAD tokens. The user removed most of his collateral but made an amount to make his position overcollateralized. since he mints small DYAD amounts he only needs to keep a small amount as collateral. The price of the collateral changes and the position is undercollateralized now. No one wants to liquidate that position, as he will pay a lot for gas compared to the value he will gain. VaultManagerV2.sol#L156-L169

// @audit no minimum amount to mint function mintDyad( ... ) 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); } This can introduce issues where some positions can be undercollateralized, which is not a good thing for the protocol and coin stability, and there is no incentivize to liquidate the position.

Recommendations Make a minimum amount for minting new DYAD when calling VaultManagerV2::mintDyad(). Make the burning failed if the position goes below the threshold you configured for minting when calling VaultManagerV2::burnDyad() .

Lines of code

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

Vulnerability details

Impact

The kerosine price is determined using the TVL, and the total Supply of the DYAD token.

Vault.kerosine.unbounded.sol#L60-L65

  function assetPrice()  ...  (uint) {
      for (uint i = 0; i < numberOfVaults; 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;
  }

What can occur is that if there is a rich person with large collaterals in a given position. He can continuously mint and burn DYAD. Making the Price Go Up and Down.

VaultManagerV2.sol#L156-L181

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

  /// @inheritdoc IVaultManager
  function burnDyad( ... ) external isValidDNft(id) {
    dyad.burn(id, msg.sender, amount);
    emit BurnDyad(id, amount, msg.sender);
  }

There are no restrictions for minting and burning. You should just keep CR > 150% when minting. So if a Rich person have a lot of collaterals in His Position and has no DYAD minted he can mint a lot of DYAD tokens.

And of course, he can also burn them, no problem.

• Minting DYAD will make kerosine price decrease.
• Burning DYAD will make kerosine price increase.

This will introduce some Attacks to occur.

1. Arbitrage trading and gaining profits.
2. Frontrun users' transactions by either increasing the price of kerosine or decreasing it.
3. Minting A large amount of DYAD to decrease kerosine prices, this will make positions that depend on kerosine as collateral (endogenous) in an under-collateralization risk.

The number of things that occur from controlling the price is a lot and we just said some of the issues that can occur.

Prood of Concept

We will explain how manipulating the kerosine price will make a profit for the manipulator.

Arbitrage

What can rich people do to make arbitrage trading profits from that issue is:

1. Burn a lot of DYAD tokens.
2. Kerosine price increases.
3. Swap that kerosine to another token (on 3rd party DEX or something).
4. Mint that DYAD tokens again and they will gain a profit as they made a swap when the kerosine price worth more than its real value.

Liquidating Positions

1. Mint a lot of DYAD tokens.
2. Kerosine price decreases.
3. Some positions, which have endogenous collaterals, go undercollateralized.
4. Liquidate these positions and gain additional rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

1. Do not allow burning from a position that is minted short time ago. We can make a MIN_BURN_AMOUNT variable to be 1 day. so the token owner can only burn his tokens if he minted them 1 day ago. This will prevent minting and burning in a short time.

2. Do not allow minting in a short period of time. This will be like if you minted now, you can only mint tomorrow.

This will not mitigate the issue 100%, but it will reduce its occurrence and impact.

Assessed type

Context

Summary

---

[L-01] BoundedKerosineVault::setUnboundedKerosineVault is not fired when deploying, leading to DoS when they are added.

For BoundedKerosineVault to return the assetPrice, it depends on the UnBoundedkerosineVault asset price, and multiply it by 2.

Vault.kerosine.bounded.sol#L44-L50

  function assetPrice() 
    public 
    view 
    override
    returns (uint) {
      return unboundedKerosineVault.assetPrice() * 2;
  }

And unboundedKerosineVault should be set by the owner of the contract.

Vault.kerosine.bounded.sol#L23-L30

  function setUnboundedKerosineVault(
    UnboundedKerosineVault _unboundedKerosineVault
  )
    external
    onlyOwner
  {
    unboundedKerosineVault = _unboundedKerosineVault;
  }

In Deploy.V2.s.sol, the vault is not set before transferring the ownership from the Deployer to the MAINNET_OWNER (multi-sig). which makes adding this vault leads to reverting when calling different functions in the VaultManagerV2, and DoS the protocol.

Deploy.V2.s.sol#L78-L91

  function run() public returns (Contracts memory) {
    ...

    UnboundedKerosineVault unboundedKerosineVault = new UnboundedKerosineVault( ... );

    BoundedKerosineVault boundedKerosineVault     = new BoundedKerosineVault( ... );

    KerosineDenominator kerosineDenominator       = new KerosineDenominator( ... );

    unboundedKerosineVault.setDenominator(kerosineDenominator);

    // @audit we did not set the unBoundedVault in `boundedKerosineVault` before transfering the ownership

    unboundedKerosineVault.transferOwnership(MAINNET_OWNER);
    boundedKerosineVault.  transferOwnership(MAINNET_OWNER);
}

PoC

• The protocol Deployed into the mainnet.
• The ownership of the boundedKerosineVault transferred to multi-sig.
• boundedKerosineVault was added as licensed vaults.
• The owner of the dNFT owners added that vault.
• VaultManagerV2::getNonKeroseneValue, VaultManagerV2::getKeroseneValue, and VaultManagerV2::getTotalUsdValue will revert when getting called, as they call getUsdValue which calls assetPrice, which will make a call to the address(0), and it will revert.
  • If the vault is added as a NonKerosene vault getNonKeroseneValue() and getTotalUsdValue() will revert.
  • If the vault is added as Kerosene vault getKeroseneValue() and getTotalUsdValue() will revert
  • If the vault is added to both then all functions will get reverted (if it is a licensed vault as kerosene vault and normal vault).

This will make all the protocols in DoS, as these functions are called when minting, withdrawing, and liquidating.

In the Deployment script, all settings and configurations occur to all newly deployed contracts before transferring the ownership to MAINNET_OWNER, So we believe that this is an issue.

Recommendations

Set the unBoundedKeroseneVault before transferring the ownership.

Deploy.V2.s.sol L:89

  function run() public returns (Contracts memory) {
    ...

    unboundedKerosineVault.setDenominator(kerosineDenominator);

+   boundedKerosineVault.setUnboundedKerosineVault(unboundedKerosineVault);

    unboundedKerosineVault.transferOwnership(MAINNET_OWNER);
    boundedKerosineVault.  transferOwnership(MAINNET_OWNER);

    ...
  }

---

[L-02] No minimum threshold for positions makes so small positions unprofitable to get liquidated

the owners of the dNFT can mint DYAD by providing collateral equal to 150%. and if his position gets undercollateralized. people can liquidate him and get a reward for that.

But there is no minimum minting limit for the position. So if the NFT owner made a position with a small amount, and after time this position gets undercollateralized. it can be unprofitable to liquidate that position as the return may be too little compared to the gas that the liquidator will pay for the calling.

POC

• The owner deposited an amount of ETH.
• The user minted some small DYAD tokens.
• The user removed most of his collateral but made an amount to make his position overcollateralized.
• since he mints small DYAD amounts he only needs to keep a small amount as collateral.
• The price of the collateral changes and the position is undercollateralized now.
• No one wants to liquidate that position, as he will pay a lot for gas compared to the value he will gain.

VaultManagerV2.sol#L156-L169

  // @audit no minimum amount to mint
  function mintDyad( ... ) 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);
  }

This can introduce issues where some positions can be undercollateralized, which is not a good thing for the protocol and coin stability, and there is no incentivize to liquidate the position.

Recommendations

• Make a minimum amount for minting new DYAD when calling VaultManagerV2::mintDyad().
• Make the burning failed if the position goes below the threshold you configured for minting when calling VaultManagerV2::burnDyad() .

---

[L-03] Maximum kerosine vaults variable is written wrongly

In KersineManager.sol, MAX_VAULTS is set to 10.

KerosineManager.sol#L14

  uint public constant MAX_VAULTS = 10;

But in VaultManagerV2, The number is set to 5.

VaultManagerV2.sol#L23

  uint public constant MAX_VAULTS_KEROSENE = 5;

This will introduce issues if they are intended to add more than 5 vaults. and having the limit number different in two different locations is not good after all.

Recommendations

Make the value the same in both, either make it 5 in both or 10 in both.

---

[L-04] Funding Exogenous vaults manipulate kerosine price

kerosine value should represent degree of DYAD’s overcollateralization. as its value is determined using the total DYAD minted and the total value Locked.

Vault.kerosine.unbounded.sol#L60-L65

  function assetPrice() ... returns (uint) {
      ...
      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;
  }

But the problem here is that balanceOf thinks that all the balance in the vault is used as collateral for DYAD.

If we donate the vaults, the donated funds will not be collateral funds, as they will be locked inside the Vault contract (can not be used to mint DYAD). This will make kerosine price deviate from the correct value it should represent (over-collateralization ratio), where the token price (kerosine) will increase thinking that there are a lot of collaterals for the stablecoin (DYAD), but in reality, these funds are locked in vaults and do not represent DYAD collaterals.

Recommendations

It can be left as it is, as there are no impacts that can occur for that in my opinion. However, if the developers are interested in mitigation, they can track the balance internally.

---

[L-05] The user can liquidate himself

In VaultManagerV2::liquidate, the function is not checking if the one he is firing liquidate is the owner of the dNFT ID to be liquidated or not.

VaultManagerV2.sol#L205-L228

  function liquidate( ... ) external isValidDNft(id) isValidDNft(to) {
      ...
  }

This can make users liquidate themselves and prevent others from getting the reward for liquidating

Recommendations

Prevent liquidation if the caller is the owner of the dNFT id position to get liquidated.

---

[L-06] The user can mint 0 DYAD

There is no restrictions for a user to mint 0 amount DYAD when firing VaultManagerV2::mintDyad().

VaultManagerV2.sol#L156-L169

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

This may introduce some misbehaviours when using the protocol.

Recommendations

Check that the amount provided is greater than 0, and providing a minimum value to deposit will be better.

---

[L-07] No way to change MAINNET_OWNER in KerosineDenominator

The denomenator is determined to be the TVL of Kerosen subtracting the Supply of the owner.

staking/KerosineDenominator.sol#L21

  function denominator() external view returns (uint) {
    // @dev: We subtract all the Kerosene in the multi-sig.
    //       We are aware that this is not a great solution. That is
    //       why we can switch out Denominator contracts.

    return kerosine.totalSupply() - kerosine.balanceOf(MAINNET_OWNER);
  } 

There is no way to change the MAINNET_OWNER in KerosineDenominator contract. So if the team wants to mitigate the funds from the MAINNET_OWNER to another address, in case of normal changing or they think MAINNET_OWNER (multisig) addresses get compromised and they want to be sure everything is safe. They will be forced to change redeploy the contract and change VaultManager configurations, which is a lot of efforts.

Recommendations

Make a function to change the MAINNET_OWNER by a trusted address.

---

[L-08] boundedKerosineVault is not added as a license vault

In Deploy.V2.s.sol, there is a comment on the line that adds boundedKerosineVault in Licences vaults.

Deploy.V2.s.sol#L96

  function run() public returns (Contracts memory) {
    ...

    vaultLicenser.add(address(ethVault));
    vaultLicenser.add(address(wstEth));
    vaultLicenser.add(address(unboundedKerosineVault));
    // @audit why commenting this
    // vaultLicenser.add(address(boundedKerosineVault));

    ...
  }

Recommendations

Remove that comment and add the vault as a Licensed one. or delete the line totally if you do not intend to add it to the licensed vault.

---

---

---

[NC-01] Unused Modifier

In VaultManagerV2, there is a modifier that checks if the vault is licenced or not.

VaultManagerV2.sol#L45-L47

  modifier isLicensed(address vault) {
    if (!vaultLicenser.isLicensed(vault)) revert NotLicensed(); _;
  }

This modifier was used in the prev Vault Manager. But as kerosine vaults introduced in V2, the team ignored using that modifier. And it is not used in the codebase.

Recommendations

Remove it from the Codebase, and this will save some GAS too when deploying.

---

[NC-02] No interface for VaultManagerV2

There is no interface for VaultManagerV2. and there are some new functions related to kerosene is added in the V2 which was not introduced in the previous VaultManager.

Recommendation

Provide a separate interface for the VaultManagerV2.

---

[NC-03] Code Structure Lacks Comments and formatting, and test suit is not the best

• The codebase has less number readable comments.
• Uses the same interface for more than one Contract which is not good.
• Code Format is not the best, behaves like C codebases, which is not the standard format in solidity.
• Testing Base is not the best, it checks only Simple and superficial things, without digging deep
• No fuzz Testing

Recommendations

Improve the codebase and make it easy for new developers to understand it.

---

---

---

Centralization risk and admin privileged functions

• Protocol Devs can change Kerosine price by calling setDenominator.

  function setDenominator(KerosineDenominator _kerosineDenominator) 
    external 
      onlyOwner
  {
    kerosineDenominator = _kerosineDenominator;
  }

• Unlicencing Vaults has a lot of impacts like position undercollateralization, and if there are no vaults DYAD can not be changed back into collaterals.

  function add   (address vault) external onlyOwner { isLicensed[vault] = true; }
  function remove(address vault) external onlyOwner { isLicensed[vault] = false; }

• setUnboundedKerosineVault can allow developers to manipulate kerosine price in BoundedkerosineVaults.

  function setUnboundedKerosineVault(
    UnboundedKerosineVault _unboundedKerosineVault
  )
    external
    onlyOwner
  {
    unboundedKerosineVault = _unboundedKerosineVault;
  }