DYAD - 0xabhay's results

Lines of code

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

Vulnerability details

Impact

The protocol's vulnerability allows for the minting of dyad tokens using only kerosene as collateral, which contravenes the intended design that prohibits using kerosene alone for this purpose. Additionally, this flaw enables users to bypass the critical 150% collateralization ratio requirement, threatening the protocol's financial stability. Exploitation of this bug could lead to the issuance of dyad tokens that are not sufficiently backed by collateral, potentially causing a devaluation of the dyad currency and jeopardizing the protocol's solvency.

<details> <summary>Proof of Concept</summary>

The deploymentscript adds the unboundedKerosineVault to the vaultLicenser, which could be problematic because the intention is that users should not be able to mint dyad solely with kerosene as collateral

a user adds the unboundedKerosineVault using VaultManagerV2::add and inside there is check

 function add(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
  ///......
    // @audit this check will passed for the unbounded vault because of the deployment script
    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
    //...
  }

and the check is passed now the user deposits 1 million kerosene using the VaultManagerV2::deposit function. As of now when i am writing this report the cost of 1 KEROSENE = 0.03456 USDC ($0.0317) 1m kerosene = 34560.3 usdc. also this can be huge if the user is taking a flash loan to exploit.

User calls mintDyad with 36500 dyad as the amount and with their address

now inside the mintdyad function it calls the getNonKeroseneValue()

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

since there is only one vault which is the unboundedkerosine vault so it will call the getUsdValue inside this function it will call the Vault.kerosine.unbounded::assetprice():

  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();
      // 1000000000e18 - 950841953.47e18 = 49158046.53e18
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
    }
}  

KerosineManager has only two vault, weth & wstETH. so we will only calculating the assetprice() of weth ans wstETH now lets take an example , suppose

tvl of weth = 500 weth,

tvl of wstETH = 500 stEth

dyad totalsupply (as of the time of writing this report) = 632967400000000000000000 (in 18 decimal)

return value of assetPrice() for Weth = 318542608000

return value of assetPrice() for wstETH = 368088673998

now calculating the tvl using the Tvl formula

numetor = 3433156409990000000000000 - 632967400000000000000000 = 2800189009990000000000000

denominator = 1000000000e18 - 950841953.47e18 = 49158046.53e18

return = 2800189009990000000000000 * 1e8 / 49158046.53e18 = 5696298 (return value of assetPrice() inside unbounded kerosine vault contract)

now, calculating the getUsdValue for Unbounded Vault,

since there is a only one vault added by the user the getNonKeroseneValue will only run for one vault,so the totalUsdValue = 56962.98e18.

now back to mintdyad the check will pass because the newDyadminted is 0 + 37000e18 (attacker is minting for the first time). and the getNonKeroseneValue(id) is 56962.98e18

now the user minted the 36500 dyad,

  dyad.mint(id, to, amount);
  if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
    emit MintDyad(id, amount, to);

after minting dyad it will call, the collatRatio(id) inside this function it will return the getTotalUsdValue because the user _dyad is non zero it is 37000e18

now inside the getTotalUsdValue it return

  function getTotalUsdValue(
    uint id
  ) 
    public 
    view
    returns (uint) {
      return getNonKeroseneValue(id) + getKeroseneValue(id);
  }

so, the getnonkerosene is 56962.98e18 and getKeroseneValue is zero becuase user didnt add any exogenous collateral.

so the return value is 56962.98e18 returned to collatration for the ratio calculation = 56962.98e18.divwadDown(36500e18) = 1.5606e18 (18 decimal)

now the check in mintDyad :This check will pass, allowing the user to successfully mint dyad. Since the collatRatio returns 1.5606 is greater than the MIN_COLLATERIZATION_RATIO, the check will pass, enabling the user to mint dyad using only kerosene tokens.

  dyad.mint(id, to, amount);
  if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
    emit MintDyad(id, amount, to);

Due to the addition of the unboundedKerosineVault to the vaultLicenser, an attacker can mint dyad tokens using only kerosene as collateral.

This attack is profitable as 1 million kerosene costs $34,560.3, and the user minted 36,500 dyad, with dyad being pegged 1:1 to stablecoins such as USDC, USDT, or DAI. Therefore, the profit from the attack would be $36,500 - $34,560.3 = $1,939.7 in stablecoin. The profitability could be significantly amplified if the exploiter utilizes a flash loan to scale the attack.

</details>

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the VaultManagerV2::add function to include additional checks that prevent unbounded vaults from being used as collateral sources for minting dyad.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L93-L94 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L67 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L80 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L156-L169

Vulnerability details

Impact

The duplication bug that allows the same exogenous vault to be counted in both getNonKeroseneValue and getKeroseneValue could lead to a significantly inflated collateral ratio, enabling users to mint more DYAD than the actual collateral should permit. potentially resulting in under-collateralization and jeopardizing user funds.

Proof of Concept

If you look at the deployment script, the team has added the exogenous vault to the Kerosene Manager and also into the Vault License. Thus, the user can add the same exogenous vault using add and addKerosene.

<details> <summary>Case 1: Using inflated collateral to mint Dyad</summary>

as per docs provide $1500 worth of wETH or wstETH, mint $1000 worth of Dyad stablecoins.

here how the user can mint more than 150% of their deposit collateral

Deposit: User deposits 10 WETH into a vault, with WETH priced at $3000 each, totaling $30000 worth of WETH.

Mint DYAD: User attempts to mint 29998 DYAD, which is just under the USD value of the deposited WETH.

The mintDyad function calls getNonKeroseneValue to ensure the vault's USD value is sufficient to cover the new DYAD minting.

  function getNonKeroseneValue(
    uint id
  ) 
    public 
    view
    returns (uint) {
      uint totalUsdValue;
      uint numberOfVaults = vaults[id].length(); 
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[id].at(i));
        uint usdValue;
        // @audit-> this check will pass for exogenous vaults becuase of the deployment script 
        if (vaultLicenser.isLicensed(address(vault))) {
          usdValue = vault.getUsdValue(id);        
        }
        totalUsdValue += usdValue;
      }
      return totalUsdValue;
  }

Calculate USD Value: getUsdValue calculates the USD value of the WETH in the vault:

For instance, if the asset price of WETH in terms of USD is $3000, the assetPrice function would return 3000e8, because the ETH/USD price is conventionally expressed with 8 decimal places.

Verify Collateralization: The contract checks whether the vault's USD value exceeds the amount of DYAD requested for minting. This check passes because getKeroseneValue returns 30000e18, which is greater than the newDyadMinted amount of 29998e18.

Minting Allowed: Since the vault value is sufficient, DYAD is minted.

Post-minting, the collatRatio is assessed: getTotalUsdValue aggregates the values from getNonKeroseneValue and getKeroseneValue. If the same WETH vault is referenced in both cases due to the user adding the same vault to both kerosene and non-kerosene sets, the total value would be 60000e18. This duplication could lead to an inflated collateral ratio calculation.

Calculate Collateral Ratio: The collatRatio function divides the total USD value by the minted DYAD amount:

60000e18 / 29998e18 ≈ 2.00013334e18

Collateral Ratio Check: The minted DYAD amount is checked against the minimum collateralization ratio (MIN_COLLATERIZATION_RATIO = 1.5e18). Since 2.00013334e18 > 1.5e18, the minting process is successful.

The user successfully mints 29998 DYAD with a collateral ratio above the required minimum, effectively allowing nearly 1:1 minting of DYAD to the USD value of the collateral.

This process relies on the assumption that both getNonKeroseneValue and getKeroseneValue reference the same Vault, which may not be the intended design. The system is designed to prevent under-collateralization, but if the same Vault is counted twice, it could lead to overestimation of collateral and potential system risk.

</details> <details> <summary>Case 2: Excessive Withdrawal</summary>

A user deposits 10 WETH into a vault. The current price of WETH is $3000, so the total collateral value is $30,000. The user mints DYAD worth $20,000, expecting to maintain a collateral ratio above the minimum of 150%.

Due to the bug, the same vault is counted in both getNonKeroseneValue and getKeroseneValue, doubling the perceived collateral value to $60,000. The user now sees a collateral ratio of 300% ($60,000 / $20,000) instead of the actual 150% ($30,000 / $20,000).

The user decides to withdraw assets based on the inflated collateral ratio.

They request a withdrawal equivalent to $10,000, expecting the system to maintain the minimum collateral ratio after the withdrawal.

However, because the actual collateral is only $30,000, after withdrawing $10,000 worth of WETH, only $20,000 worth of WETH remains to back the $20,000 of minted DYAD.

The actual collateral ratio post-withdrawal is now 100% ($20,000 / $20,000), which is below the required 150%.

This leaves the system under-collateralized and vulnerable to price fluctuations that could further diminish the collateral's value.

</details>

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a check in add and addKerosene functions to verify that a vault is not already present in the other set before adding.

For the add function:

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

For the addKerosene function:

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 (vaults[id].contains(vault))                         revert VaultAlreadyInNonKerosene();
  if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
  emit Added(id, vault);
}

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The exploitation of the block.number dependency bug can lead to a denial of service where legitimate users are unable to withdraw their funds. This undermines the trust in the system and can lead to financial losses and reputational damage for the platform.

Proof of Concept

The VaultManagerV2::deposit lacks a check to ensure that the caller is the DNft holder. This omission could potentially result in a serious problem.

This can be manupilated by miner or non-miner lets go through the one by one example

Here's how a miner could execute this attack:

Monitor Mempool: The miner monitors the mempool for withdraw transactions related to a specific id.

Prepare Deposit: Upon detecting a withdraw transaction, the miner prepares a deposit transaction for the same id.

// @audit no check anyone can deposit for a valid id even if they are not the owner of DNft
  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
       
  {
    
    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

The miner can use any account to perform this deposit, including their own.

Construct Block: When constructing the next block, the miner includes their own deposit transaction before the user's withdraw transaction.

Mine Block: The miner mines the block, ensuring that their deposit transaction is included first.

Publish Block: The miner publishes the block to the blockchain.

Withdrawal Fails: When the block is confirmed, the withdraw transaction is processed after the deposit transaction. The withdraw transaction fails because the check in VaultManagerV2::withdraw

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

now matches the current block number, triggering the DepositedInSameBlock revert condition.

Here's a step-by-step explanation of how a non-miner might carry out this attack:

Monitor Mempool: The attacker monitors the mempool for withdraw transactions related to a specific id using a Web3 provider or blockchain explorer API.

Craft Deposit Transaction: Upon detecting a withdraw transaction, the attacker quickly creates a deposit transaction for the same id.

Increase Gas Price: The attacker sets a higher gas price for their deposit transaction to incentivize miners to prioritize and include it in the next block before the pending withdraw transaction.

Broadcast Deposit Transaction: The attacker broadcasts their deposit transaction to the network.

Miners Include Deposit First: Miners, motivated by the higher gas fees, include the attacker's deposit transaction in the next block before the withdraw transaction.

Withdrawal Fails: When the withdraw transaction is processed, it fails because the check in VaultManagerV2::withdraw

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

matches the current block number due to the inclusion of the attacker's deposit transaction, triggering the DepositedInSameBlock revert condition.

Repeat: The miner and non-miner can continue this strategy for subsequent blocks, effectively preventing the user from withdrawing by ensuring that a deposit transaction for the same id always precedes any withdraw transaction in the block order

Tools Used

Manual Review

Recommended Mitigation Steps

allow deposits only from the owner of the DNft, ensure that the VaultManagerV2::deposit function includes a check to verify the caller's ownership status.

  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
    isValidDNft(id)
++  isDNftOwner(id)
    
  {

    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The lack of proper ownership verification in the deposit function could lead to significant security vulnerabilities. An attacker could exploit this oversight to deposit assets into unauthorized or malicious vaults using any DNft ID, regardless of ownership. This could result in the unauthorized use of the platform, manipulation of vault balances, and potential loss of user funds.

Proof of Concept

Identify or deploy a malicious vault contract that could, for example, redirect deposited assets to an attacker-controlled address.

Without owning a particular DNft, call the deposit function with its ID and the address of the malicious vault, along with the amount to deposit. Since there is no ownership check,

//@audit-> Anyone can deposit for any other ID, even if they do not hold the corresponding NFT
// attacker can cause issue by creating a non lincensed vault and deposit with the valid Dnft
  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {

    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    //@audit-> lacks the Licensed  vault check
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

the transaction could succeed, allowing the attacker to deposit assets into the malicious vault.

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the deposit function to include the isDNftOwner modifier and Licensed vaults check for both kerosene and non kerosene.

function deposit(
  uint id,
  address vault,
  uint amount
) 
  external 
    isValidDNft(id)
++  isDNftOwner(id)
{
++  if (!vaultLicenser.isLicensed(vault) && !keroseneManager.isLicensed(vault)) revert NotLicensed();

  idToBlockOfLastDeposit[id] = block.number;
  Vault _vault = Vault(vault);
  _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
  _vault.deposit(id, amount);
}

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The potential underflow in the Vault.kerosine.unbounded::assetPrice() function can lead to a denial of service where users are unable to withdraw their assets from the Unbounded kerosine vault. This can be exploited by an attacker to manipulate the TVL, potentially causing financial loss to users.This could lead to loss of confidence in the system and financial loss for users if they are unable to access their assets when needed.

Proof of Concept

In Deployment Script a new WETH or wstETH vault is created without any initial assets, resulting in a TVL of zero. Additionally, the deployment script grants an unbounded vault to the vault licenser, so the unbounded vault can be added to the VaultManagerV2 using the add function. This also satisfies the check of !vaultLicenser.isLicensed(vault).

<details> <summary>Case 1: When the TVL < dyad.totalSupply()</summary> user add the Unbouded kerosine vault using `VaultManagerV2::add`

A user deposits assets into the Unbounded kerosine vault.

The user attempts to withdraw, triggering the vault.assetPrice() function

 function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
///........
    uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() // oracle decimal is 8
                  / 10**_vault.asset().decimals();
                  ///......
  }                  

Since the user is withdrawing from the Unbounded Kerosene vault, the assetPrice() function specific to the Unbounded vault is called. This function's calculation depends on the TVL of the exogenous vaults (e.g., WETH, wstETH), which can be influenced by external users.

  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());
      }
      //@audit-> underflow reverts when tvl < totalsupply
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
    }

Since the TVL is zero and the dyad total supply is greater than zero, the subtraction in the assetPrice calculation results in an underflow. Even if a user deposits WETH and wstETH, for a withdrawal to succeed, it must be ensured that the TVL is greater than dyad.totalSupply(). If the TVL is less than dyad.totalSupply() after a user's deposit, the transaction will fail, and the user will not be able to withdraw.

</details> <details> <summary> Case 2: When the TVL > dyad.totalSupply() </summary>

User Prepares Withdrawal: A user initiates a withdrawal from a unbounded vaultwith a significant TVL in exogenous Vault.

Attacker Monitors: An attacker, holding a large portion of the assets in theexogenous Vault, monitors the network for pending withdrawal transactions.

Attacker Front-Runs: Upon detecting a user's withdrawal transaction, the attacker submits their own withdrawal from the exogenousVault. and submit a transaction with a higher gas fee to ensure it gets processed first.

Attacker Withdraws Assets: The attacker's withdrawal transaction is confirmed first due to the higher gas fee, significantly reducing the TVL in the vault. now TVL < dyad.totalSupply()

User's Withdrawal Transaction Processes: When the user's withdrawal transaction is processed, it triggers the Vault.assetPrice() ex(unbounded vault) function.

TVL Falls Below Dyad Total Supply: The attacker's withdrawal may have caused the TVL to fall below the dyad total supply.

Underflow in Asset Price Calculation: The assetPrice function calculates a negative value due to underflow, as the TVL is now less than the dyad total supply.

Transaction Reverts: The smart contract reverts the user's withdrawal transaction because of the underflow, preventing the user from retrieving their assets and effectively locking them in the unboundedVault.

In both scenarios, the attacker exploits the underflow vulnerability in the assetPrice function, which assumes that the TVL will always be greater than the dyad total supply. This vulnerability can lead to a denial of service for users trying to withdraw their assets.

</details> <details> <summary>Case 3: When the TVL == dyad.totalSupply() </summary>

here's a example of how the TVL could be manipulated to equal dyad.totalSupply(), causing the assetPrice to return zero and triggering a failure in the withdrawal process due to the NotEnoughExoCollat check, we would follow these steps:

Initial State: Assume the contract has a certain tvl (total value locked) and dyad.totalSupply() that are not equal. The assetPrice is therefore greater than zero.

Deposit/Withdraw Manipulation: An actor (could be a user or a group) starts interacting with the exogenous vaults that contribute to the tvl calculation. They could either deposit more assets into the exogenous vaults or withdraw assets from them.

Equalizing TVL and Dyad Supply: The actor continues to adjust their deposits and withdrawals until the tvl exactly equals dyad.totalSupply(). This could be done by monitoring the contract state off-chain and performing transactions accordingly.

TVL Equals Dyad Supply: Once tvl is equal to dyad.totalSupply(), the numerator in the assetPrice calculation becomes zero tvl - dyad.totalSupply() = 0.

Asset Price Calculation: When the assetPrice function is called, it returns zero because the numerator is zero, making the price calculation 0 * 1e8 / denominator = 0.

Withdrawal Attempt: A user tries to withdraw their funds from the unbounded vault. The withdrawal function calls getNonKeroseneValue(id), which relies on assetPrice of vault whhich is unbounded vault here and to determine the USD value of the assets that are not part of the Kerosene collateral.

Zero Asset Price Impact: Since assetPrice is zero, getNonKeroseneValue(id) also returns zero. This means that the system believes there is no non-Kerosene collateral value.

NotEnoughExoCollat Check: The withdrawal function then performs a check to ensure that the user has enough exogenous collateral to cover the withdrawal after accounting for the Kerosene collateral (dyadMinted). The check is if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat;.

Revert Condition Met: Because getNonKeroseneValue(id) returns zero, any non-zero value requested for withdrawal will cause the subtraction to underflow (which is prevented by Solidity 0.8.x), or it will simply be less than dyadMinted, thus triggering the NotEnoughExoCollat revert.

Withdrawal Failure: The transaction is reverted, and the user is unable to withdraw their funds, effectively locking assets in the vault until the issue is resolved.

</details>

NOTE: This could also affect the redeemDyad function in the same way it affects the withdraw function. Users may not be able to redeemDyad because the underflow could cause the transaction to revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Validation Checks: Implement validation checks in the assetPrice function to ensure that the TVL is always greater than or equal to dyad.totalSupply(). If not, the function should return a meaningful error rather than allowing an underflow.

Case 3 solution:

  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();
++    require(tvl > 0 , "tvl should be greater than zero");  
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

This vulnerability can be exploited to avoid liquidations, allowing under-collateralized positions to persist and potentially leading to insolvency risks for the vault. It undermines the integrity of the liquidation mechanism, which is crucial for maintaining the platform's financial health.

Proof of Concept

In Deployment Script a new WETH or wstETH vault is created without any initial assets, resulting in a TVL of zero. Additionally, the deployment script grants an unbounded vault to the vault licenser, so the unbounded vault can be added to the VaultManagerV2 using the add function. This also satisfies the check of !vaultLicenser.isLicensed(vault).

a user has minted dyad worth $2000 using 1 ETH as collateral at a price of $3000 per ETH. Additionally, the user has deposited 1 Kerosene into the unbounded vault. If the user's collateralization ratio (CR) falls below the minimum required CR, their position becomes eligible for liquidation.

Here's how the vulnerability can prevent liquidation:

Liquidation Trigger: The user's position becomes under-collateralized, triggering eligibility for liquidation.

Liquidation Process: Another party or the system itself initiates the liquidation process by calling the liquidate function.

Collateral Valuation: The VaultManagerV2::liquidate function calls collatRatio -> getTotalUsdValue(id) to determine the total USD value of the user's collateral.

Calling getNonKeroseneValue(id): Within getTotalUsdValue(id), the getNonKeroseneValue(id) function is called to assess the value of non-Kerosene collateral, which is a unbounded vault here.

Underflow in assetPrice: If the TVL of the unbounded vault is less than dyad.totalSupply, the assetPrice calculation for the unbounded vault underflows, potentially reverting the transaction.

The underflow causes the transaction to revert, preventing liquidation

If the TVL Total Value Locked of the UnboundedKerosineVault is less than dyad.totalSupply(), the calculation of assetPrice() could underflow when subtracting dyad.totalSupply() from tvl. This underflow would cause the transaction to revert. Since TVL is influenced by external users through deposits and withdrawals, it can be manipulated by any user.

Tools Used

Manual Review

Recommended Mitigation Steps

If tvl is less than dyad.totalSupply(), return a minimum asset price value that represents an under-collateralized position, triggering liquidation.

Assessed type

Context