setUnboundedKerosineVault not called during deployment, causing reverts when querying for Kerosene value after adding it as a Kerosene vault

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L78-L82
https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.bounded.sol#L23-L30
https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/README.md?plain=1#L66-L68

Vulnerability details

Root Cause

The setUnboundedKerosineVault function was never called during deployment, nor was it planned to be called post deployment.

Impact

Without setting the unboundedKerosineVault, any attempt to get the asset price of a dNFT that has uses the bounded Kerosene vault will result in a revert.

Note Regarding Vault Licenser

VaultManagerV2's addKerosene() erroneously does a vault license check using keroseneManager.isLicensed(vault) at VaultManagerV2.sol#L88 making it impossible to add Kerosene vaults to Notes. As clarified by the sponsor in this video DYAD V2- Kerosene - Code4rena Audit #2, the vaults in keroseneManager are intended to be used for kerosene value calculation and kerosene vaults are not supposed to be added there. We updated the relevant Kerosene vault license checks to use vaultLicenser.isLicensed(vault) instead as it is aligned with the deployment script at Deploy.V2.s.sol#L95 since unboundedKerosineVault is added as a licensed vault with vaultLicenser.add(address(unboundedKerosineVault))

The two following code changes were made to VaultManagerV2.sol so that the unbounded kerosene vault can be added as a kerosene vault without further changes in the vaults, the vault manager, or the deployment script.

VaultManagerV2.sol#L88
From:

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

To:

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

and VaultManagerV2.sol#L280
From:

if (keroseneManager.isLicensed(address(vault))) {

To:

if (vaultLicenser.isLicensed(address(vault))) {

Proof of Concept

The following test script demonstrates that the getKeroseneValue function reverts when the unboundedKerosineVault is not set during deployment.

forge t --mt test_boundedVaultValueRevert --fork-url <MAINNET_RPC_URL> -vv

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import "forge-std/Test.sol";

import {DeployV2, Contracts} from "../../script/deploy/Deploy.V2.s.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Parameters} from "../../src/params/Parameters.sol";
import {WETH} from "../WETH.sol";
import {DNft} from "../../src/core/DNft.sol";

contract V2TestBoundedKeroseneVault is Test, Parameters {

  Contracts contracts;

  function setUp() public {
    contracts = new DeployV2().run();
  }

  function test_boundedVaultValueRevert() public {
    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    address alice = makeAddr("alice");
    vm.prank(MAINNET_OWNER);
    uint aliceTokenId = DNft(MAINNET_DNFT).mintInsiderNft(alice);    

    // drop 1000 eth into ethVault for initial TVL used for calculating kerosene price
    vm.deal(address(contracts.ethVault), 1000 ether);
    vm.prank(address(contracts.ethVault));
    WETH(payable(MAINNET_WETH)).deposit{value: 1000 ether}();

    // add boundedKerosineVault as a licensed vault since it was commented out in the deploy script
    vm.prank(MAINNET_OWNER);
    contracts.vaultLicenser.add(address(contracts.boundedKerosineVault));

    // add boundedKerosineVault to kerosene vault
    vm.prank(alice);
    contracts.vaultManager.addKerosene(aliceTokenId, address(contracts.boundedKerosineVault));

    // getKeroseneValue now reverts
    vm.expectRevert();
    contracts.vaultManager.getKeroseneValue(aliceTokenId);

    // set the unbounded kerosine vault for the bounded kerosine vault
    vm.prank(MAINNET_OWNER);
    contracts.boundedKerosineVault.setUnboundedKerosineVault(contracts.unboundedKerosineVault);

    // this is fine now
    contracts.vaultManager.getKeroseneValue(aliceTokenId);

  }
}

Tools Used

Manual testing

Recommended Mitigation Steps

Set the unboundedKerosineVault during deployment.

Changes to DeployV2

Call the setUnboundedKerosineVault function during deployment after deploying the bounded Kerosene vault at Deploy.V2.s.sol#L78-L82:

boundedKerosineVault.setUnboundedKerosineVault(unboundedKerosineVault);

Assessed type

Other

[c4-pre-sort]

JustDravee marked the issue as high quality report

[c4-pre-sort]

JustDravee marked the issue as primary issue

[shafu0x]

doesn't necessarily needs to be called in the deployment script

[c4-judge]

koolexcrypto marked the issue as unsatisfactory:
Invalid

[McCoady]

While the sponsor comment is true, the documentation in the contest's README explicitly states:
The whole migration is described in Deploy.V2.s.sol. The only transaction that needs to be done by the multi-sig after the deployment is licensing the new Vault Manager..

This finding shows that this is in fact not the case and that the comments in the provided documentation suggest the team were unaware this would be an issue. Given the deploy script is within the scope of the contest I believe this issue is a valid finding. If this issue had not been raised and the protocol had deployed as they previously outlined, users who deposit would have their funds stuck (due to both the withdraw and mintDyad functions reverting) until the DYAD team themselves worked out what the issue was and called the necessary function.

[koolexcrypto]

Thanks for your input.

The statement in README is about the migration from VaultManager to VaultManagerV2.

users who deposit would have their funds stuck (due to both the withdraw and mintDyad functions reverting)

Not sure how the funds will be stuck if the UnboundedKerosineVault is not set. UnboundedKerosineVault is used in BoundedKerosineVault to retrieve the price. So, BoundedKerosineVault will not function till this is set. Furthermore, withdraw is disallowed in BoundedKerosineVault.

[McCoady]

Not sure how the funds will be stuck if the UnboundedKerosineVault is not set. UnboundedKerosineVault is used in BoundedKerosineVault to retrieve the price. So, BoundedKerosineVault will not function till this is set. Furthermore, withdraw is disallowed in BoundedKerosineVault.

Funds will be stuck because the value of all a users collateral is checked on withdraw (not just the collateral they're attempting to withdraw) in the collatRatio(id) call.
Therefore if they have added the bounded kerosene vault to their vaults mapping the withdraw function will revert when attempting to calculate the value of their bounded kerosene.

[InfectedIsm]

Consider L04 from my QA report if this report is being validated: #980

[koolexcrypto]

Thank you for your further explanation.

After reviewing README and the comments above again, because of

1- There is an impact (clarified already by the warden) that will make the protocol not function.
2- The statement in README

The whole migration is described in Deploy.V2.s.sol. The only transaction that needs to be done by the multi-sig after the deployment is licensing the new Vault Manager

I believe, it would be unfair to mark this as QA, will upgrade to Medium.

[c4-judge]

koolexcrypto removed the grade

[c4-judge]

koolexcrypto marked the issue as selected for report