c4-bot-7 commented 6 months ago

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.