Errors and missing code in Deployment script leads to miscalculation in Kerosene value and DOS in adding Kerosene vault for Dnft Users

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L80-L91 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.bounded.sol#L44-L50 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L64-L65

Vulnerability details

Description

The deployment script improperly configures KerosineManager by registering non-kerosene vaults (ethVault and wstEth) instead of the intended kerosene-related vaults (UnboundedKerosineVault and BoundedKerosineVault). This misconfiguration leads to multiple functional issues:

1. Addition of Kerosene Vaults: The addKerosene function in VaultManagerV2, dependent on KerosineManager for licensing verification, fails because the necessary kerosene vaults are not correctly licensed. This prevents legitimate kerosene vault additions, essential for protocol operations.

2. Asset Price Calculation: The BoundedKerosineVault is not properly configured to reference the UnboundedKerosineVault, causing its assetPrice() function, which depends on the unbounded vault, to fail due to an unset reference.

3. Collateralization and Token Minting: Due to the flawed' KerosineManager' setup, the getKeroseneValue function erroneously includes values from non-kerosene vaults. This impacts the calculation of collateralization ratios and the proper minting of tokens, this would lead to users being able to mint DYAD based on wrong collateral ratio assumptions.

These issues compromise the protocol’s functionality and financial integrity and could lead to significant fund loss for the protocol and users alike.

Proof of Concept

To dive deeper, first we need to understand how the KerosineManager works:

Inside KerosineManager, we have:

uint public constant MAX_VAULTS = 10;
EnumerableSet.AddressSet private vaults;

And

    function getVaults() external view returns (address[] memory) {
        return vaults.values();
    }

    function isLicensed(address vault) external view returns (bool) {
        return vaults.contains(vault);
    }

Where vaults is the AddressSet of all the Kerosene assets (bounded and non-bounded), this is evident from the following two functions in VaultManagerV2.sol:

1. addKerosine() : Link for reference
   • This method links a Kerosene Vault to a particular dNft.
2. getKeroseneValue : Link for reference
   • This method returns the total USD value of Kerosene inside Bounded/unbounded vaults.

Both of these methods make an external call to KeroseneManager to check whether the kerosene vault is licensed via:

keroseneManager.isLicensed(address(vault)

Given this context and no additional documentation/natspec for KeroseneManager stating otherwise, we can be sure that vaults contain kerosene vaults.

Now we have the following issues:

• The deployment script incorrectly adds ethVault and wstEth to KerosineManager, which should only contain kerosene vaults. This can be seen in the script:

  kerosineManager.add(address(ethVault));
  kerosineManager.add(address(wstEth));

• The correct kerosene vaults, UnboundedKerosineVault and BoundedKerosineVault, are not added to KerosineManager, leading to failed licensing checks in the addKerosene function:

  function addKerosene(uint id, address vault) external isDNftOwner(id) {
    if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed();
  }

• BoundedKerosineVault does not have the UnboundedKerosineVault set, which is critical for its assetPrice() calculation:

  function assetPrice() public view override returns (uint) {
    return unboundedKerosineVault.assetPrice() * 2;
  }

• getKeroseneValue will also give wrong values, as the vaults added to keroseneManager are ethVault and wstEth and boundedKeroseneVault and unboundedKeroseneVault are left out.

  • This function is supposed to return the total value of all Kerosene-related assets, not the exogenous ones.

    function getKeroseneValue(uint id) public view returns (uint) {
    uint totalUsdValue;
    uint numberOfVaults = vaultsKerosene[id].length();
    for (uint i = 0; i < numberOfVaults; i++) {
    Vault vault = Vault(vaultsKerosene[id].at(i));
    uint usdValue;
    if (keroseneManager.isLicensed(address(vault))) {
        usdValue = vault.getUsdValue(id);
    }
    totalUsdValue += usdValue;
    }
    return totalUsdValue;
    }

    GitHub Links:

• addKerosene()

• assetPrice()

• deploymentScript

Alternate Assumption:

Even though I have provided sufficient proof via codebase implementation to assert why vaults in KerseneManager should contain Kerosene vaults. But let's assume that sponsors simply made a typo. Instead of checking keroseneManager.isLicensed(vault), they wanted to check vaultLicenser.isLicensed(vault) or maybe no check at all (which in itself will be a critical vulnerability).

In such a case, we would have the following scenario:

1. KeroseneManager will contain a list of exogenous vaults, thus its assetPrice() will work because it will call the following method of Vault.sol, and it should work fine

   function assetPrice() public view returns (uint) {
   (, int256 answer, , uint256 updatedAt, ) = oracle.latestRoundData();
   if (block.timestamp > updatedAt + STALE_DATA_TIMEOUT)
       revert StaleData();
   return answer.toUint256();
   }

2. However, now the getKerosene method will fail, because we won't have any kerosene vaults in the keroseneManager

   function getKeroseneValue(uint id) public view returns (uint) {
   uint totalUsdValue;
   uint numberOfVaults = vaultsKerosene[id].length();
   for (uint i = 0; i < numberOfVaults; i++) {
       Vault vault = Vault(vaultsKerosene[id].at(i));
       uint usdValue;
       if (keroseneManager.isLicensed(address(vault))) { <<< @audit can't have any kerosene vault in keroseneManager
           usdValue = vault.getUsdValue(id);
       }
       totalUsdValue += usdValue;
   }
   return totalUsdValue;
   }

3. This would mean that getTotalUsdValue and collatRatio will also fail. If collatRatio fails, then withdraw will fail, and users' deposits will forever be stuck!

Impact

The misconfiguration leads to a denial of service (DoS) for functionalities that rely on adding kerosene vaults or calculating asset prices in BoundedKerosineVault. This could severely impact operational capabilities, financial calculations, and the overall reliability of the system.

Tools Used

• Solidity: The smart contracts are written in Solidity.
• Forge: Assumed use for scripting deployments.
• Manual Review: Analyzing the deployment script and related smart contracts.

Recommended Mitigation Steps

1. Correct the KerosineManager Configuration:

   • Modify the deployment script to ensure that only kerosene-related vaults are managed by KerosineManager. Remove the lines where non-kerosene vaults are added and ensure the correct kerosene vaults are included:

     kerosineManager.add(address(unboundedKerosineVault));
     kerosineManager.add(address(boundedKerosineVault));

2. Configure BoundedKerosineVault Properly:

   • Update the deployment script to set the UnboundedKerosineVault for the BoundedKerosineVault immediately after both are instantiated:

     boundedKerosineVault.setUnboundedKerosineVault(unboundedKerosineVault);

Assessed type

Other

[c4-pre-sort]

JustDravee marked the issue as duplicate of #966

[c4-pre-sort]

JustDravee marked the issue as sufficient quality report

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid

[c4-judge]

koolexcrypto marked the issue as duplicate of #1133

[c4-judge]

koolexcrypto marked the issue as satisfactory