DYAD - 0xAsen's results

Lines of code

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

Vulnerability details

Impact

The current implementation allows anyone to deposit on behalf of someone else's DNft. However, this can be used by a malicious actor to DOS functions like withdraw() or remove().

Proof of Concept

As you can see, anyone can deposit for any vault, given that the id exists:

function deposit(uint id, address vault, uint amount) external isValidDNft(id)
  {
    //updating the block of last deposit
    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

Now, let's take a look at the withdraw function:

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

What could happen here is that a malicious actor can frontrun a normal users' withdraw transaction with a dust deposit, update the idToBlockOfLastDeposit[id] variable and therefore DOS the withdrawal because of this check:

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

This could be done by a malicious actor either by frontrunning or simply by depositing at every possible block. This won't be expensive at all since he can just keep depositing 1 wei of the asset and that is enough.

There's a similar issue in the remove and removeKerosene functions as well:

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

To remove a vault, the vault should be empty of all assets. However, a malicious actor can just keep depositing 1 wei of assets effectively DOSing the function.

Tools Used

Manual review

Recommended Mitigation Steps

Either make the deposit function with stricter access control allowing just DNft owners to deposit for their vaults. Or at least add a reasonable minDeposit amount to make such an attack not worth it for an attacker.

Assessed type

Access Control