c4-bot-1 commented 3 months ago

Lines of code

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

Vulnerability details

Impact

The exploitation of the block.number dependency bug can lead to a denial of service where legitimate users are unable to withdraw their funds. This undermines the trust in the system and can lead to financial losses and reputational damage for the platform.

Proof of Concept

The VaultManagerV2::deposit lacks a check to ensure that the caller is the DNft holder. This omission could potentially result in a serious problem.

This can be manupilated by miner or non-miner lets go through the one by one example

Here's how a miner could execute this attack:

Monitor Mempool: The miner monitors the mempool for withdraw transactions related to a specific id.

Prepare Deposit: Upon detecting a withdraw transaction, the miner prepares a deposit transaction for the same id.

// @audit no check anyone can deposit for a valid id even if they are not the owner of DNft
  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 miner can use any account to perform this deposit, including their own.

Construct Block: When constructing the next block, the miner includes their own deposit transaction before the user's withdraw transaction.

Mine Block: The miner mines the block, ensuring that their deposit transaction is included first.

Publish Block: The miner publishes the block to the blockchain.

Withdrawal Fails: When the block is confirmed, the withdraw transaction is processed after the deposit transaction. The withdraw transaction fails because the check in VaultManagerV2::withdraw

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

now matches the current block number, triggering the DepositedInSameBlock revert condition.

Here's a step-by-step explanation of how a non-miner might carry out this attack:

Monitor Mempool: The attacker monitors the mempool for withdraw transactions related to a specific id using a Web3 provider or blockchain explorer API.

Craft Deposit Transaction: Upon detecting a withdraw transaction, the attacker quickly creates a deposit transaction for the same id.

Increase Gas Price: The attacker sets a higher gas price for their deposit transaction to incentivize miners to prioritize and include it in the next block before the pending withdraw transaction.

Broadcast Deposit Transaction: The attacker broadcasts their deposit transaction to the network.

Miners Include Deposit First: Miners, motivated by the higher gas fees, include the attacker's deposit transaction in the next block before the withdraw transaction.

Withdrawal Fails: When the withdraw transaction is processed, it fails because the check in VaultManagerV2::withdraw

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

matches the current block number due to the inclusion of the attacker's deposit transaction, triggering the DepositedInSameBlock revert condition.

Repeat: The miner and non-miner can continue this strategy for subsequent blocks, effectively preventing the user from withdrawing by ensuring that a deposit transaction for the same id always precedes any withdraw transaction in the block order

Tools Used

Manual Review

Recommended Mitigation Steps

allow deposits only from the owner of the DNft, ensure that the VaultManagerV2::deposit function includes a check to verify the caller's ownership status.

  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 3 months ago

JustDravee marked the issue as duplicate of #489

c4-pre-sort commented 3 months ago

JustDravee marked the issue as sufficient quality report