DYAD - BiasedMerc's results

Lines of code

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

Vulnerability details

Impact

VaultManagerV2::deposit contains the isValidDnft modifier, which only ensures that a DNft is valid, not that the owner is calling the deposit function. The issue lies in the fact that any user can call deposit with any DNft and update idToBlockOfLastDeposit[id] = block.number.

This can be used to block the DNft owners withdrawal of funds, as the check in withdraw will revert as the DNft has had a deposit in the same block by the malicious user. The cost for this attack is gas fees and the protocol has stated it will be deploying on ETH, therefore this is why risk is medium.

Proof of Concept

VaultManagerV2::deposit

  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
    idToBlockOfLastDeposit[id] = block.number;

The deposit function contains the isValidDNft modifier:

VaultManagerV2::isValidDNft

  modifier isValidDNft(uint id) {
    if (dNft.ownerOf(id) == address(0))   revert InvalidDNft(); _;
  }

The modifier checks that the dNft has a non-zero address, i.e it's been minted. Meaning that any user can deposit funds for any valid dNft, depositing is not restricted to only the owner.

idToBlockOfLastDeposit is utilised to stop flashloans attack by using a locking mechanism, where once an dNft has a deposit in a block, that block.number is stored. When calling VaultManagerV2::withdraw this variable is checked against the current block number, and the function reverts if a deposit has been completed within this block:

VaultManagerV2::withdraw

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

The issue arises from the fact that this lock is not user/ address specific, meaning that after a deposit has been made within one block for a dNft, any attemp to withdraw will revert due to the check above.

This can leads to withdrawals reverting due to random deposits within the same block before the withdrawal call is included in the block. Another issue is that a malicious user can monitor withdrawal attempts for specific dNfts, and ensure that a dust deposit using that dNft is placed before the withdrawal within the same block, causing the withdrawal to revert. This can theoretically be repeated forever, causing the owner of the dNft to never be able to withdraw their funds. In reality this attack would cost gas fees, so this attack would be costly to be conducted for a long time.

Tools Used

Manual review

Recommended Mitigation Steps

One solution to this issue is to only allow a dNft owner to deposit assets linked to their own dNft. This would ensure that the idToBlockOfLastDeposit[id] can only be activated by the dNft owner.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

For a vault to be removed from a Dnft, it needs to have no assets left within the specific vault tied to the Dnft id. However due to the fact that anyone can deposit funds for any Dnft ID, a malicious user can front-run an owners call to remove and DOS the vault removal indefinitely.

Proof of Concept

A Dnft owner can remove added vaults by calling VaultManagerV2::remove():

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

The first check ensures that no assets are left within the vault that are linked to the Dnft ID. Therefore a normal function call flow to remove a Dnft would be for the owner to first call VaultManagerV2::withdraw() with the full balance and then to call VaultManagerV2::remove().

However, the deposit function allows ANY user to add funds (with no minimum amount) to any Dnft's vault. Meaning a malicious griefer can front-run the owners remove() call and add a dust amount of funds. This will cause the call to revert as there will be leftover funds tied to the Dnft.

VaultManagerV2::deposit()

  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 only validation check is ensuring that the Dnft ID is valid, however there is no owner check.

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure only the Dnft owner can deposit funds, this will give the owner full control of the Dnft and vault balance tied to the Dnft. This also fixes a different bug related to withdrawal DOS.

Assessed type

DoS