DYAD - d_tony7470's results

Lines of code

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

Vulnerability details

Summary

When the user makes a deposit, this variable idToBlockOfLastDeposit[id] takes a value block.number:

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

When withdrawing, there is a check that reverts if idToBlockOfLastDeposit[id] == block.number. That was made to prevent flashloan attack (when deposit and withdraw in same block). This can be used by an attacker to DOS withdrawing user's assets: he can frontrun withdrawing transaction by making a call deposit(victimsId, vault, 0) on behalf of user, thus idToBlockOfLastDeposit[id] == block.number will be true. It is possible due to lack of access control in deposit() function.

Impact

DOSing of withdraw() function.

Tools Used

Manual review.

Recommended Mitigation Steps

Make sure that msg.sender == dNft.ownerOf(id):

 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

Access Control

Lines of code

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

Vulnerability details

Summary

Attacker can prevent removing vaults for anyone by depositing on behalf of user id. The vaults contained in the vaults[id] array are very important for the user, because they are used to determine prices and collatRatio. The protocol allows users to create an array of 10 vaults and edit it. Editing is possible using the functions remove():

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

The vulnerability is in this check: if (!vaults[id].remove(vault)) revert VaultNotAdded(); An attacker can exploit this by making a small deposit(1 wei) through the deposit() function, where there is no check to see if the depositor is isDNftOwner(id):

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

function deposit(
    uint id,
    uint amount
  )
    external 
      onlyVaultManager
  {
    id2asset[id] += amount;
    emit Deposit(id, amount);
  }

As a result, it will increase the value of id2asset[id], making removing not possible, because the check (Vault(vault).id2asset(id) > 0) will always be true.

Impact

DOSing of remove() function.

Tools Used

Manual review.

Recommended Mitigation Steps

Make sure that msg.sender == dNft.ownerOf(id):

 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

Access Control