DYAD - turvy_fuzz's results

Lines of code

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

Vulnerability details

Impact

Attacker can prevent owners from withdrawing without actually depositing any amount since vault address isn't validated and can be any address including the attacker own custom vault contract. Therefore at zero cost.

Proof of Concept

The withdraw() function implements a mechanism to prevent flashloan attack issue by the Id last deposited when withdrawing:

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

This last deposit mapping is updated on the 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);
  }

Notice from the above function shown, there where no access control or permission required to deposited for any valid Id, also notice there are no license check for the vault to be deposited. This means an attacker can prevent owners from withdrawing without actually depositing any amount since vault address isn't validated and can be any address including the attacker own custom vault contract, and since there are no permission needed, can frontrun whenever attempts to withdraw and update the lastdeposited to the same block.number as the withdraw transaction.

Tools Used

Manual review

Recommended Mitigation Steps

Implement an access control mechanism to allow only addresses with the owner's permission/allowance.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Attackers can prevent DNftOwners from removing vaults from their Id.

Proof of Concept

Here are the remove functions:

 function remove(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
@>  if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
    if (!vaults[id].remove(vault))     revert VaultNotAdded();
    emit Removed(id, vault);
  }

  function removeKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
@>  if (Vault(vault).id2asset(id) > 0)     revert VaultHasAssets();
    if (!vaultsKerosene[id].remove(vault)) revert VaultNotAdded();
    emit Removed(id, vault);
  }

Notice they both check if the vault still has assets in them before allowing them to be removed. These assets can added to the vault through the deposit function with any amount even as little as 1 unit of the token:

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 issue here is that as we can see, the deposit function has no access control meaning anyone can add access to the vault including when an owner attempts to remove the vault after withdrawing, frontrunning the transition and depositing an insignificant amount to prevent the owner from removing vaults.

Tools Used

Manual Review

Recommended Mitigation Steps

Either consider allowing owners to choose to remove vault even if assets are in them, they can still be added back or implement an access control or permission allowance from the Id owners to deposit for them.

Assessed type

DoS