User can get their Kerosene stuck because of an invalid check on withdraw

criticalCode4rena

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

solidity📋
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