DYAD - itsabinashb's results

Lines of code

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

Vulnerability details

Impact

The current logic of VaultManagerV2::add() allows Kerosene vaults to be added and VaultManagerV2.adddKerosene() allows wethVault/wstEthVault to be added. But only wethVault & wstEthVault should be added by add() and keroseneVaults should be added by addKerosene().

Proof of Concept

The VaultManagerV2::add() looks like this:

File: VaultManagerV2.sol
76:   function add(uint id, address vault) external isDNftOwner(id) {
77:     // @note For adding Normal vault
78:     if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
79:     if (!vaultLicenser.isLicensed(vault)) revert VaultNotLicensed();
80:     if (!vaults[id].add(vault)) revert VaultAlreadyAdded(); 
81:     emit Added(id, vault);
82:   }

You can see there is only 1 check for Vault type, which is license check, as UnboundedKeroseneVault is licensed by VaultLicenser so it will easily be added here.

Now let's have a look at VaultManagerV2::addKerosene():

File: VaultManagerV2.sol
88:   function addKerosene(uint id, address vault) external isDNftOwner(id) {
89:     if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults(); 
90:     if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed();
91:     if (!vaultsKerosene[id].add(vault)) revert VaultAlreadyAdded();
92:     emit Added(id, vault);
93:   }

As wethVault & wstWethVault are licensed by KeroseneManager they will be easily added here.

Tools Used

Manual review.

Recommended Mitigation Steps

Replace this check: if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed(); with vaultLicenser check in add() & remove the keroseneManager check from addKerosene(). In addKerosene() one check can given which is checking that whether the asset of the vault address is Kerosene or not.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

As per the rule of this protocol one can can't withdraw in same block in which he deposited. For ex- if an user deposited in 23423411 block then he can't withdraw in same block, he will have to wait until next block come. However, an malicious user can take this advantage and revert an valid withdrawal by depositing asset for that user's NFT id. In VaultManagerV2::deposit() a modifier was used: isValidNft(id) which checks that whether the owner of that NFT id is address(0) or not, it is visible that anyone can call this deposit() to deposit asset for a valid NFT id, this is intended too. But in VaultManagerV2::withdraw() there is a check:

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

Means you can't withdraw in same block. So what attacker can do is, when he see a transaction for withdraw in mempool he will front that transaction by depositing 1 wei of asset, for example 1 wei of WETH, so as the idToBlockOfLastDeposit[id] = block.number; is updated in deposit() with new block number the withdrawal will revert.

Proof of Concept

Replace the BaseTest.sol file with this:

<details> <summary>Click me</summary>

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

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

contract BaseTest is Test, Parameters {
DNft         dNft;
Licenser     vaultManagerLicenser;
Licenser     vaultLicenser;
Dyad         dyad;
VaultManagerV2 vaultManager;
Payments     payments;

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

// dai
Vault        daiVault;
ERC20Mock    dai;
OracleMock   daiOracle;

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

  Contracts memory contracts = new DeployBase().deploy(
    msg.sender,
    address(dNft),
    address(weth),
    address(wethOracle), 
    GOERLI_FEE,
    GOERLI_FEE_RECIPIENT
  );

  vaultManagerLicenser = contracts.vaultManagerLicenser;
  vaultLicenser        = contracts.vaultLicenser;
  dyad                 = contracts.dyad;
  vaultManager         = contracts.vaultManager;
  wethVault            = contracts.vault;

  // create the DAI vault
  dai       = new ERC20Mock("DAI-TEST", "DAIT");
  daiOracle = new OracleMock(1e6);
  daiVault  = new Vault(
    vaultManager,
    ERC20(address(dai)),
    IAggregatorV3(address(daiOracle))
  );

  // add the DAI vault
  vm.prank(vaultLicenser.owner());
  vaultLicenser.add(address(daiVault));
}

receive() external payable {}

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

</details>

Replace the DeployBase.s.sol script with this:

<details> <summary>Click me</summary>

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

import "forge-std/Script.sol";
import {DNft}          from "../../src/core/DNft.sol";
import {Dyad}          from "../../src/core/Dyad.sol";
import {Licenser}      from "../../src/core/Licenser.sol";
import {VaultManagerV2}  from "../../src/core/VaultManagerV2.sol";
import {Vault}         from "../../src/core/Vault.sol";
import {Payments}      from "../../src/periphery/Payments.sol";
import {IAggregatorV3} from "../../src/interfaces/IAggregatorV3.sol";
import {IWETH}         from "../../src/interfaces/IWETH.sol";

import {ERC20} from "@solmate/src/tokens/ERC20.sol";

// only used for stack too deep issues
struct Contracts {
 Licenser     vaultManagerLicenser;
 Licenser     vaultLicenser;
 Dyad         dyad;
 VaultManagerV2 vaultManager;
 Vault        vault;
}

contract DeployBase is Script {

 function deploy(
   address _owner, 
   address _dNft,
   address _asset,
   address _oracle, 
   uint    _fee,
   address _feeRecipient
 )
   public 
   payable 
   returns (
     Contracts memory
   ) {
     DNft dNft = DNft(_dNft);

     vm.startBroadcast();  // ----------------------

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

     Dyad dyad                     = new Dyad(
       vaultManagerLicenser
     );

     VaultManagerV2 vaultManager     = new VaultManagerV2(
       dNft,
       dyad,
       vaultLicenser
     );

     Vault vault                   = new Vault(
       vaultManager,
       ERC20(_asset),
       IAggregatorV3(_oracle)
     );

     // 
     vaultManagerLicenser.add(address(vaultManager));
     vaultLicenser       .add(address(vault));

     //
     vaultManagerLicenser.transferOwnership(_owner);
     vaultLicenser       .transferOwnership(_owner);

     vm.stopBroadcast();  // ----------------------------

     return Contracts(
       vaultManagerLicenser,
       vaultLicenser,
       dyad,
       vaultManager,
      vault
     );
 }
}

</details>

Now create file in test directory and paste this:

// SPDX-License-Identifier: MIT
pragma solidity >=0.6.0;

import './BaseTest.sol';
import 'forge-std/Test.sol';
import '../src/interfaces/IVaultManager.sol';
import {ERC20Mock} from './ERC20Mock.sol';

contract RevertWithdraw is BaseTest {
  address attacker;
  address user;

  function set() public {
    attacker = vm.addr(0x123);
    user = vm.addr(0x456);
    deal(address(weth), address(user), 10e18);
    deal(address(weth), address(attacker), 10e18);
    vm.deal(user, 10 ether);
  }

  function test_frontRun() public {
    set();
    vm.startPrank(user);

    dNft.mintNft{value: 1 ether}(address(user));
    deposit(weth, 0, address(wethVault), 10e18);
    vaultManager.mintDyad(0, 1, address(user));
    vm.stopPrank();
    vm.roll(2);
    vm.startPrank(attacker);
    weth.approve(address(vaultManager), 10);
    //! Attacker front run the user's withdraw call and deposited 1 wei of WETH
    vaultManager.deposit(0, address(wethVault), 1);
    vm.stopPrank();
    vm.prank(user);
    vm.expectRevert(IVaultManager.DepositedInSameBlock.selector);
    vaultManager.withdraw(0, address(wethVault), 5e18, address(user));
  }

  function deposit(ERC20Mock token, uint id, address vault, uint amount) private {
    vaultManager.add(id, vault);
    token.approve(address(vaultManager), amount);
    vaultManager.deposit(id, address(vault), amount);
  }
}

Run this test with: forge test --mt test_frontRun

Tools Used

Manual review, Foundry.

Recommended Mitigation Steps

Put a limit of deposit, at least no one can deposit 1 wei of WETH.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The Vault.kerosene.unbounded::withdraw() is supposed to get called by VaultManagerV2 contract. Whenever a user want to withdraw his Kerosene he will call the VaultManagerV2::withdraw(), as kerosene can be withdrawn only from unbounded kerosene vault so, the user need to pass the unbounded kerosene vault address to the VaultManagerV2::withdraw() as vault address. One important thing to note here that, the value of kerosene is deterministic means it is determined in Vault.kerosene.unbounded::assetPrice(), protocol does not use any oracle to get the price of kerosene. Now take a closer look at this VaultManagerV2::withdraw():

  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
    uint dyadMinted = dyad.mintedDyad(address(this), id);
    Vault _vault = Vault(vault);
    uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() 
                  / 10**_vault.asset().decimals();
    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

Let assume unbounded kerosene vault address was passed as vault argument, so in this line: Vault _vault = Vault(vault) the contract initializes the _vault local variable with the unbounded kerosene vault contract address. After that, in this line: / 10**_vault.oracle().decimals() the oracle is called. But there no oracle was used in any of 2 kerosene vaults, so this call will revert, means user can't withdraw his kerosene token ever.

Proof of Concept

1. The value of kerosene is deterministic: https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/Vault.kerosine.unbounded.sol#L50-L68

2. No oracle was used in any of kerosene vault contracts: i. https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.sol ii. https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol iii. https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.bounded.sol

Tools Used

Manual review.

Recommended Mitigation Steps

Remove the oracle call from accounting when vault address is unbounded kerosene vault.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

One of main invariant of protocol is TVL > Dyad total supply, but this invariant does not hold true always. The kerosene price is calculated in unbounded kerosene vault, that time TVL is calculated. For instance, when there are total 50*1e18 asset in total (WETH & wstETH) that time the invariant does not hold true while result underflow panic revert.

Proof of Concept

At the time of writing this report the WETH oracle returns 326582208201 price & stETH oracle returns 325594269867 price. stETH/USD => https://etherscan.io/address/0xCfE54B5cD566aB89272946F602D76Ea879CAb4a8#readContract#F8 ETH/USD => https://etherscan.io/address/0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419#readContract#F8 To ease our calculation we will take only WETH price & assume this price for both ethVault & wstEthVault. We know the Kerosene balance of MAINNET_OWNER is 6729475100942126514132000 & total supply of Kerosene is 1000000000000000000000000000. The total supply of DYAD is: 632967400000000000000000. So lets calculate the kerosene denominator:

➜ 1000000000000000000000000000 - 6729475100942126514132000
Type: uint256
├ Hex: 0x000000000000000000000000000000000000000003359d370c4e3c884c14abe0
├ Hex (full word): 0x000000000000000000000000000000000000000003359d370c4e3c884c14abe0
└ Decimal: 993270524899057873485868000

Now take a look at formula of TVL calculation:

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

Here, dev is looping through all vaults which were licensed by kerosene manager, for now there are only 2 vaults, so I will take same amount of asset for 2 vaults, which is 25e18 amount & multiply it by 2, so we will get the value for 2 vaults.

➜ 25*1e18 * uint(326582208201)*1e18/10**18/10**8
Type: uint256
├ Hex: 0x00000000000000000000000000000000000000000000114a03a5950738fe6400
├ Hex (full word): 0x00000000000000000000000000000000000000000000114a03a5950738fe6400
└ Decimal: 81645552050250000000000
➜ 81645552050250000000000 * 2  // @audit multiplying by 2 because there are 2 vaults
Type: uint256
├ Hex: 0x000000000000000000000000000000000000000000002294074b2a0e71fcc800
├ Hex (full word): 0x000000000000000000000000000000000000000000002294074b2a0e71fcc800
└ Decimal: 163291104100500000000000

So TVL is 163291104100500000000000. Now if we do, TVL - DYAD_totalSupply:

163291104100500000000000 - 632967400000000000000000 = −4.696762959×10²³

So we got negative value here, that means numnerator = −4.696762959×10²³, as numerator is uint256 type it will revert.

This invariant does not hold true even when total asset amount in ethVault & wstEthVault is 150e18.

➜  150*1e18 * uint(326582208201)*1e18/10**18/10**8
Type: uint256
├ Hex: 0x0000000000000000000000000000000000000000000067bc15e17e2b55f65800
├ Hex (full word): 0x0000000000000000000000000000000000000000000067bc15e17e2b55f65800
└ Decimal: 489873312301500000000000

And, TVL - Dyad total supply:

489873312301500000000000 - 632967400000000000000000 = −1.430940877×10²³

So it is negative here too.

Tools Used

Manual review, Chisel.

Recommended Mitigation Steps

I am not sure what will work good, as while withdrawing kerosene the assetPrice will be calculated, it will create issue. Will look forward to check the fix.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

This could be intended but the current implementation returning the USD value of 1 KEROSENE token for unbounded kerosene vault is 0 even when total deposited amount in both vaults(ethVault & wstETHVault) is 9000e18.

Proof of Concept

We assume the price of WETH & stETH is: 326582208201, this is latest price of WETH at the time of writing this report, and the price of stETH is 325594269867. So, we will go with 326582208201 price for ease of our calculation as both price are very close. We know, kerosene.totalSupply = 1000000000e18 and, kerosene balance of MAINNET_OWNER = 950841953470486444914439000 So, denominator = 1000000000e18 - 950841953470486444914439000 = 49158046529513555085561000. DYAD total supply = 622967400000000000000000. Imagine we have 9000 weth in both vaults. So, lets calculate the price of 1 kerosene token for unbounded kerosene vault:

➜ 9000*1e18*uint(326582208201)*1e18/10**18/10**8
Type: uint256
├ Hex: 0x00000000000000000000000000000000000000000018501520d9922825bca000
├ Hex (full word): 0x00000000000000000000000000000000000000000018501520d9922825bca000
└ Decimal: 29392398738090000000000000
➜ 29392398738090000000000000 - 622967400000000000000000
Type: uint256
├ Hex: 0x00000000000000000000000000000000000000000017cc29ff7624e67c18a000
├ Hex (full word): 0x00000000000000000000000000000000000000000017cc29ff7624e67c18a000
└ Decimal: 28769431338090000000000000
➜ uint(28769431338090000000000000)*1e8/uint(49158046529513555085561000)
Type: uint256
├ Hex: 0x00000000000000000000000000000000000000000000000000000000037d02c6
├ Hex (full word): 0x00000000000000000000000000000000000000000000000000000000037d02c6
└ Decimal: 58524358

And if we calculate the USD value of this then it will be:

➜ uint usdValue = 1*uint(58524358)/1e8;
➜ usdValue
Type: uint256
├ Hex: 0x0
├ Hex (full word): 0x0
└ Decimal: 0

So, you can see the USD value of 1 kerosene token is 0.

Tools Used

Manual analysis, Chisel.

Recommended Mitigation Steps

As I already said it could be intended but if it is not then a legit fix required.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The VaultManagerV2::addKerosene() is intended to add kerosene vaults, currently there are only 2 types of kerosene vaults: 1. Bounded kerosene vault & 2. Unbounded kerosene vault. However, in the addKerosene() there is a check:

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

As per protocol need only 2 vaults were licensed by KeroseneManager contract: 1. Vault.sol i.e ethVault & 2. VaultWstETH.sol i.e wstETHVault. In KeroseneManager.sol contract a vault can only be licensed if it is added in KeroseneManager contract, by calling the KeroseneManager::add(). These 2 vaults were added here because protocol wants to account only these 2 vaults to calculate TVL. So, When the addKerosene() will be called with one of those [ bounded or unbounded kerosene vault] kerosene vault's address it will revert, because kerosene vaults were not added in KeroseneManager contract so it was not licensed.

Proof of Concept

The reason of the revert:

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L88C4-L88C87

Only 2 vaults were licensed by KeroseneManger contract:

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/script/deploy/Deploy.V2.s.sol#L64-L65

Protocol does not want to account the kerosene vaults while calculating the TVL:

https://youtu.be/ok4CBaqEajM?si=e7D4XySF5CUTliko&t=377

Vaults will only be licensed by KeroseneManager contract if it is added by calling KeroseneManager::add():

1. https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/KerosineManager.sol#L50
2. https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/KerosineManager.sol#L25

Tools Used

Manual review.

Recommended Mitigation Steps

Remove this line of code from VaultManagerV2::addKerosene():

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

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L280

Vulnerability details

Impact

To understand the issue we will have to understand how collateral ratio is calculated. To calculate the collateral ratio protocol calculates the USD value of 2 positions: 1. For non kerosene vaults and 2. for kerosene vaults.

Now take a looks how kerosene vaults are added:

1. First user calls the VaultManagerV2::addKerosene() with any of two [bounded or unbounded] kerosene vault address.
2. After that the mapping(uint => EnumerableSet.AddressSet) internal vaultsKerosene mapping is updated with the vault address i.e the vault address is added in vaultsKerosene mapping.

While calculating the collateral ratio the VaultManagerV2 contract calculates the USD value for both of positions, as i already mentioned above. One important thing to note here is, any of 2 kerosene vault contracts are not licensed by kerosene manager contract because the protocol does not want to account kerosene vault contracts while calculating TVL. Now take a closer look to getKeroseneValue():

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

Notice the if block:

if (keroseneManager.isLicensed(address(vault))) {
          usdValue = vault.getUsdValue(id);        
        }

If the vault address is licensed by KeroseneManager contract then only update the usdValue. Now remember how kerosene vaults were added, it was added in vaultsKerosene mapping, so when in getKeroseneValue(), in for loop vaults will be fetched from the mapping, all vaults will be kerosene vaults, because only kerosene vaults were added in that mapping. As the kerosene vaults were not licensed by KeroseneManager contract the usdValue will not be updated here, so it's value will remain 0 and for that the totalUsdValue will be 0, so getKeroseneValue() will return 0.

In VaultManagerV2::collatRatio(), to calculate collateral ratio, protocol divides the total usd value of all positions by minted DYAD for that NFT id. Total usd value is calculated in VaultManagerV2::getTotalUsdValue(), by doing: getKeroseneValue() + getNonKeroseneValue(). Here, for kerosene vaults we got 0 as kerosene value.

As we know the minimum collateralization ratio is 150%. So, due to 0 value returned by getKeroseneValue() the accounting of collateral ratio may go below 150% and for that a healthy position will be liquidated.

Additionally this can create issue during mintDyad() call because in this function the collateral ratio is checked.

File: src/core/VaultManagerV2.sol
167:     if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();

Proof of Concept

1. Kerosene vaults are not licensed by KeroseneManager contract because protocol does not want to account kerosene vaults while calculating TVL: https://youtu.be/ok4CBaqEajM?si=2tb8PZSeCWQm8JX5&t=377 You can see only Vault.sol and Vault.wsteth.sol is licensed by kerosene manager: https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/script/deploy/Deploy.V2.s.sol#L64-L65

Tools Used

Manual review.

Recommended Mitigation Steps

Remove the license check from for loop in VaultManagerV2::getKeroseneValue().

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

As price of Kerosene highly depends on the amount of weth & wstEth in ethVault & wstEthVault, any user can deposit big amount of weth, for example 50 weth, and increase the price of kerosene instantly.

Proof of Concept

In unbounded kerosene vault contract the assetPrice of kerosene is calculated like that:

      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;

here we assume the price of WETH & stETH is: 326582208201, this is latest price of WETH at the time of writing this report, and the price of stETH is 325594269867. So, we will go with 326582208201 price for ease of our calculation as both price are very close. We know, kerosene.totalSupply = 1000000000e18 and, kerosene balance of MAINNET_OWNER = 950841953470486444914439000 So, denominator = 1000000000e18 - 950841953470486444914439000 = 49158046529513555085561000. DYAD total supply = 622967400000000000000000. Now assume there are already 200 WETH in both vault in total. So the Kerosene price:

➜ 200*1e18*uint(326582208201)*1e18/10**18/10**8
Type: uint256
├ Hex: 0x000000000000000000000000000000000000000000008a501d2ca839c7f32000
├ Hex (full word): 0x000000000000000000000000000000000000000000008a501d2ca839c7f32000
└ Decimal: 653164416402000000000000
➜ 653164416402000000000000 - 622967400000000000000000 // @audit this is TVL - DYAD_total_supply
Type: uint256
├ Hex: 0x000000000000000000000000000000000000000000000664fbc93af81e4f2000
├ Hex (full word): 0x000000000000000000000000000000000000000000000664fbc93af81e4f2000
└ Decimal: 30197016402000000000000
➜ uint(30197016402000000000000)*1e8/uint(49158046529513555085561000)   // @audit dividing by kerosene_denominator
Type: uint256
├ Hex: 0x000000000000000000000000000000000000000000000000000000000000eff4
├ Hex (full word): 0x000000000000000000000000000000000000000000000000000000000000eff4
└ Decimal: 61428

Now Kerosene price is: 61428.

Now attacker deposited 50 WETH, which increased the asset amount to 250 WETH. Let's calculate the Kerosene price for this amount:

➜ 250*1e18*uint(326582208201)*1e18/10**18/10**8
Type: uint256
├ Hex: 0x00000000000000000000000000000000000000000000ace42477d24839efe800
├ Hex (full word): 0x00000000000000000000000000000000000000000000ace42477d24839efe800
└ Decimal: 816455520502500000000000
➜ 816455520502500000000000 - 622967400000000000000000  // @audit this is TVL - DYAD_total_supply
Type: uint256
├ Hex: 0x0000000000000000000000000000000000000000000028f903146506904be800
├ Hex (full word): 0x0000000000000000000000000000000000000000000028f903146506904be800
└ Decimal: 193488120502500000000000
➜ uint(193488120502500000000000)*1e8/uint(49158046529513555085561000)  // @audit dividing by kerosene_denominator
Type: uint256
├ Hex: 0x0000000000000000000000000000000000000000000000000000000000060184
├ Hex (full word): 0x0000000000000000000000000000000000000000000000000000000000060184
└ Decimal: 393604

And if we see the difference between both prices:

➜ 393604 - 61428
Type: uint256
├ Hex: 0x0000000000000000000000000000000000000000000000000000000000051190
├ Hex (full word): 0x0000000000000000000000000000000000000000000000000000000000051190
└ Decimal: 332176

After depositing 50 weth price increased to: 332176. So this is huge difference, by just depositing 50 weth the user increased the Kerosene price almost 5 times of previous price.

As bounded kerosene price depends in unbounded kerosene's price, twice of unbounded kerosene price, attacker can manipulate(make up & down as of his need) the price by depositing/withdrawing WETH and one can withdraw WETH easily after work done as depositing weth just need an DNFT. The vault manager has a restriction on withdrawal where withdrawal is not allowed in same block, however it will not save the price of kerosene from manipulating, user can increase the price by depositing and can withdraw on next block.

Tools Used

Manual review, Chisel.

Recommended Mitigation

Protocol should put a limit on deposit so that no one can deposit big amount at once, or protocol make the deposit 2 step process so that the attacker can't front-run to get advantage.

Assessed type

Other