c4-bot-3 commented 5 months ago

Lines of code

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

Vulnerability details

Impact

Attackers can DoS withdraw() and redeemDyad() by depositing on behalf of id with any dummy tokens.

Proof of Concept

Users can withdraw() or redeemDyad() their assets as long as the collateral ratio does not fall below 150%. The protocol implemented flashloan protection by verifying idToBlockOfLastDeposit[id] against the current block number.

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

However, this approach introduces a vulnerability where malicious users can exploit withdraw() and redeemDyad() to repeatedly fail. By executing deposit() on behalf of another id, using an arbitrary vault address, and deploying a customized vault contract, attackers can set _vault.asset to a dummy token of no value. The _vault.deposit() function can be left empty.

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

By front-running innocent users' withdraw() or redeemDyad() calls within the same block, the attacker sets idToBlockOfLastDeposit[id] to the current block. Consequently, the calls always revert. This attack results in DoS and gas grief for users.

  function redeemDyad(
    uint    id,
    address vault,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
    returns (uint) { 
      dyad.burn(id, msg.sender, amount);
      Vault _vault = Vault(vault);
      uint asset = amount 
                    * (10**(_vault.oracle().decimals() + _vault.asset().decimals())) 
                    / _vault.assetPrice() 
                    / 1e18;
      withdraw(id, vault, asset, to);
      emit RedeemDyad(id, vault, amount, to);
      return asset;
  }

Users who are not able to redeemDyad() are open to liquidation risk due to asset price fluctuation.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider not allowing other to deposit() on behalf of others.

  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

c4-pre-sort commented 5 months ago

JustDravee marked the issue as duplicate of #489

c4-pre-sort commented 5 months ago

JustDravee marked the issue as sufficient quality report