Unable to withdraw Kerosene from `vaultmanagerv2::withdraw` as it expects a `vault.oracle()` method which is missing in Kerosene vaults

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact VaultManagerV2 has one withdraw function responsible for withdrawing both exogenous collateral (weth/wsteth) and endogenous collateral (Kerosene). However the function expects the vault passed as an argument to have an oracle method. This is the case for Vault contracts, but not the case for the BoundedKerosineVault or UnboundedKerosineVault contracts. This means that whenever a user attempts to withdraw Kerosene deposited into the contract the call will revert, meaning the Kerosene remains stuck in the contract permanently.

Proof Of Concept Add the following test to v2.t.sol to highlight this.

  function testCannotWithdrawKero() public {
    // Set up alice
    licenseVaultManager();
    address alice = makeAddr("alice");
    uint aliceTokenId = sendNote(alice);
    sendKerosene(alice, 10_000 ether);

    // Alice deposits kerosene into the protocol
    vm.startPrank(alice);
    contracts.vaultManager.addKerosene(aliceTokenId, address(contracts.unboundedKerosineVault));
    Kerosine(MAINNET_KEROSENE).approve(address(contracts.vaultManager), 10_000 ether);
    contracts.vaultManager.deposit(aliceTokenId, address(contracts.unboundedKerosineVault), 10_000 ether);

    assertEq(ERC20(MAINNET_KEROSENE).balanceOf(alice), 0);

    vm.roll(block.number + 42);

    // Alice attempts to withdraw her kerosene but the tx reverts
    contracts.vaultManager.withdraw(aliceTokenId, address(contracts.unboundedKerosineVault), 10_000 ether, alice);
  }

The test reverts with the following stack traces:

    ├─ [9243] VaultManagerV2::withdraw(645, UnboundedKerosineVault: [0x416C42991d05b31E9A6dC209e91AD22b79D87Ae6], 10000000000000000000000 [1e22], alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6])
    │   ├─ [558] 0xDc400bBe0B8B79C07A962EA99a642F5819e3b712::ownerOf(645) [staticcall]
    │   │   └─ ← [Return] alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]
    │   ├─ [2623] 0x305B58c5F6B5b6606fb13edD11FbDD5e532d5A26::mintedDyad(VaultManagerV2: [0xA8452Ec99ce0C64f20701dB7dD3abDb607c00496], 645) [staticcall]
    │   │   └─ ← [Return] 0
    │   ├─ [261] UnboundedKerosineVault::asset() [staticcall]
    │   │   └─ ← [Return] 0xf3768D6e78E65FC64b8F12ffc824452130BD5394
    │   ├─ [262] 0xf3768D6e78E65FC64b8F12ffc824452130BD5394::decimals() [staticcall]
    │   │   └─ ← [Return] 18
    │   ├─ [214] UnboundedKerosineVault::oracle() [staticcall]
    │   │   └─ ← [Revert] EvmError: Revert
    │   └─ ← [Revert] EvmError: Revert

Recommended Mitigation Given that the value of exogenous and endogenous collateral is calculated differently it is necessary to handle withdrawal of exogenous collteral and Kerosene differently. It would avoid added complexity to the function logic to have two different withdraw and withdrawKerosene functions.

Assessed type

Other

[c4-pre-sort]

JustDravee marked the issue as duplicate of #1048

[c4-pre-sort]

JustDravee marked the issue as not a duplicate

[c4-pre-sort]

JustDravee marked the issue as primary issue

[c4-pre-sort]

JustDravee marked the issue as high quality report

[shafu0x]

Good find. This is correct.

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report

[Brivan-26]

This issue should be a dup or partial-50 of #397 .

This issue talks only about the missing oracle function on the kerosene tokens, the return value of the oracle call is used only to calculate the value variable which isused only on the unnecessary check that is highlighted on issue #397.

So, the root reason for calling the kerosene oracle (which will be removed because the price of Kerosene is not determined by the market) is just a preparation for making the unnecessary check highlighted on #397

[koolexcrypto]

Hi @Brivan-26

Thank you for your input.

This issue is about the missing oracle.

397 is about this check

if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat(); which prevents withdrawing kerosene unless you have non-kerosene value.

Completely different root causes. So this stays the same.