DYAD - y4y's results

Lines of code

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

Vulnerability details

Impact

An NFT owner's withdrawal can be persistently DoS'd by depositing 0 tokens.

Proof of Concept

The withdraw functions only lets the NFT owner to withdraw assets do the operation, and immediately checks if there are deposits at the same block, and reverts if so.

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

At the same time, we see in deposit function:

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

idToBlockOfLastDeposit is updated each time there is a deposit, the only requirement to call this function is by providing a valid NFT id. Since there is no minimum amount of deposited, which makes the cost of depositing extremely low.

When a malicious actor observes the withdraw action, they can immediately front the transaction by depositing 0 tokens to the correspond id, and make the transaction revert. To some extreme extend, this DoS can even make the entire vault dysfunctional as the only way to withdraw from the vaults is through the vault managers.

Tools Used

Manual review

Recommended Mitigation Steps

Add a minimum deposit amount, or allow a minimum amount withdrawable for each id.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The getKeroseneValue or getUsdValue function will potentially fail and revert if any of the Kerosine vaults are in the vaults list.

Proof of Concept

VaultManagerV2.getTotalUsdValue computes the USD value in both Kerosine and non-Kerosine vaults altogether. In both functions, vault.getUsdValue(id); is called to get single vault's price, in Kerosine vault, we can see the function is defined in Vault.kerosine.sol:

  function getUsdValue(
    uint id
  )
    public
    view 
    returns (uint) {
      return id2asset[id] * assetPrice() / 1e8;
  }

which calls assetPrice, which is implemented in the child contracts. For example, in unbounded Kerosine vault:

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

And we see, from all vaults in the KerosineManager, assetPrice of each vault is called, and combined with vault.asset and vault.oracle.

The issue is, in both bounded and unbounded Kerosine contracts, oracle is not defined, and when other functions reference this variable, it will revert as the variable doesn't exsist in those contracts. This causes all functions which may potentially call Kerosine vault's assetPrice function to revert. One example would be VaultManagerV2.liquidate:

  function liquidate(
    uint id,
    uint to
  ) 
    external 
      isValidDNft(id)
      isValidDNft(to)
    {
      uint cr = collatRatio(id);
      // ... snip

Where:

  function collatRatio(
    uint id
  )
    public 
    view
    returns (uint) {
      uint _dyad = dyad.mintedDyad(address(this), id);
      if (_dyad == 0) return type(uint).max;
      return getTotalUsdValue(id).divWadDown(_dyad);
  }

Which eventually calls assetPrice.

Tools Used

Manual review

Recommended Mitigation Steps

Add oracle variable in Kerosine contracts.

Assessed type

Context