DYAD - Mahmud's results

Lines of code

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

Vulnerability details

Impact

When calling the withdraw function, VaultManagerV2 calculates the value of the assets being withdrawn based on the asset's price and the oracle's decimals. This logic assumes that all vaults have an oracle variable, which is not the case for KerosineVault contracts such as UnboundedKerosineVault. Kerosene vaults do not define an oracle as prices are calculated deterministically, and thus the call to _vault.oracle().decimals() will fail and revert the transaction.

Proof of Concept

The following POC demonstrates the issue.

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

import "forge-std/Test.sol";
import {VaultManagerV2}         from "../../src/core/VaultManagerV2.sol";
import {DNft}                   from "../../src/core/DNft.sol";
import {Dyad}                   from "../../src/core/Dyad.sol";
import {Licenser}               from "../../src/core/Licenser.sol";
import {KerosineManager}        from "../../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../../src/core/Vault.kerosine.unbounded.sol";
import {Kerosine}               from "../../src/staking/Kerosine.sol";

contract POC is Test {
    VaultManagerV2 public vaultManager;
    uint public id;
    UnboundedKerosineVault public unboundedKerosineVault; 
    function setUp() public {
        Licenser vaultLicenser = new Licenser();
        Dyad dyad = new Dyad(vaultLicenser);
        DNft dNft = new DNft();
        Kerosine kerosene = new Kerosine();
        KerosineManager kerosineManager = new KerosineManager();
        vaultManager = new VaultManagerV2(
            dNft,
            dyad,
            vaultLicenser
        );
        unboundedKerosineVault = new UnboundedKerosineVault(
            vaultManager,
            kerosene, 
            dyad,
            kerosineManager
        );
        kerosineManager.add(address(unboundedKerosineVault));
        vaultManager.setKeroseneManager(kerosineManager);
        id = dNft.mintNft{value: 0.1 ether}(address(this));
        kerosene.approve(address(vaultManager), 5 ether);
        vaultManager.addKerosene(id, address(unboundedKerosineVault));
        vaultManager.deposit(id, address(unboundedKerosineVault), 5 ether);
    }

    function testWithdrawReverts() public {
        vm.roll(2);
        vaultManager.withdraw(id, address(unboundedKerosineVault), 5 ether, address(this));
    }

    function onERC721Received(
    address,
    address,
    uint256,
    bytes calldata
  ) external pure returns (bytes4) {
    return 0x150b7a02;
  }

    receive() external payable {} 
}

Result:

Ran 1 test for test/POC.t.sol:POC
[FAIL. Reason: EvmError: Revert] testWithdrawReverts() (gas: 32979)
Traces:
  [32979] POC::testWithdrawReverts()
    ├─ [0] VM::roll(2)
    │   └─ ← ()
    ├─ [20729] VaultManagerV2::withdraw(0, UnboundedKerosineVault: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 5000000000000000000 [5e18], POC: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   ├─ [2557] DNft::ownerOf(0) [staticcall]
    │   │   └─ ← POC: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]
    │   ├─ [2623] Dyad::mintedDyad(VaultManagerV2: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], 0) [staticcall]
    │   │   └─ ← 0
    │   ├─ [261] UnboundedKerosineVault::asset() [staticcall]
    │   │   └─ ← Kerosine: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9]
    │   ├─ [249] Kerosine::decimals() [staticcall]
    │   │   └─ ← 18
    │   ├─ [214] UnboundedKerosineVault::oracle() [staticcall]
    │   │   └─ ← EvmError: Revert
    │   └─ ← EvmError: Revert
    └─ ← EvmError: Revert

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 6.01ms (367.01µs CPU time)

Ran 1 test suite in 18.83ms (6.01ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/POC.t.sol:POC
[FAIL. Reason: EvmError: Revert] testWithdrawReverts() (gas: 32979)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Foundry

Recommended Mitigation Steps

Add a separate function for withdrawing kerosene that doesn't call oracle and doesn't do any nonKeroseneValue check as it is not needed to withdraw kerosene.

function withdrawKerosene(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L88 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L269 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L56

Vulnerability details

Vulnerability Details

In VaultManagerV2 contract, the addKerosene function is meant to be used to add kerosenevaults into the vaultsKerosene mapping. However, kerosenevaults(bounded/unbounded) won't be added as there is a check to see if that vault has been added into keroseneManager (the isLicensed call) and per the deployment script(Deploy.V2.s.sol), the two vaults added to the kerosenemanager are normal WETH and WSTETH vaults not kerosene vaults even though a bounded and an unbounded keroseneVault was created, therefore in the getKeroseneValue function which is supposed to calculate the USD value of kerosene stored in the keroseneVaults, it won't accurately compute the kerosenevalue but exogeneous value(i.e: non-kerosene collateral value).

Impact

The current implementation leads to an inaccurate representation of kerosene and exogenous(non-kerosene collateral) vaults in the VaultManagerV2, affecting the overall system's integrity as there will be an inaccurate representation the Collateralization Ratio which is a core aspect of the system. and the correct calculation of asset prices in kerosene vaults.

Proof of Concept

• The addKerosene function in VaultManagerV2 checks for licensing against keroseneManager: VaultManagerV2.sol#addKerosene
• Deployment script adds non-kerosene vaults to keroseneManager: [DeployV2.sol#run](Deployment script adds non-kerosene vaults to keroseneManager: DeployV2.sol#run)

Tools Used

Manual Review

Recommended Mitigation Steps

only add kerosene vaults into keroseneManager and add normal exogeneous vaults into the vaultManagerV2. If this is implemented, the UnboundedKerosineVault contract should use vaultManager.getVaults in the assetPrice function to get the totalValueLocked(tvl) in the exogenous vaults instead of keroseneManager.getVaults and thus there will be a need for a new enumerableSet state variable that stores all vaults added to the vaultmangerV2, vaults should be added to it when the add function is called(there won't be duplicates since it is a set) and then the vaultManager.getVaults just returns the vaults stored in that variable. DeployV2.s.sol:

VaultWstEth wstEth = new VaultWstEth(
      vaultManager, 
      ERC20        (MAINNET_WSTETH), 
      IAggregatorV3(MAINNET_CHAINLINK_STETH)
    );

    KerosineManager kerosineManager = new KerosineManager();

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

    vaultManager.setKeroseneManager(kerosineManager);

    kerosineManager.transferOwnership(MAINNET_OWNER);

    UnboundedKerosineVault unboundedKerosineVault = new UnboundedKerosineVault(
      vaultManager,
      Kerosine(MAINNET_KEROSENE), 
      Dyad    (MAINNET_DYAD),
      kerosineManager
    );

    BoundedKerosineVault boundedKerosineVault     = new BoundedKerosineVault(
      vaultManager,
      Kerosine(MAINNET_KEROSENE), 
      kerosineManager
    );
+ kerosineManager.add(address(unboundedKerosineVault));
+ kerosineManager.add(address(boundedKerosineVault));

Vault.kerosine.unbounded.sol:

function assetPrice() 
    public 
    view 
    override
    returns (uint) {
      uint tvl;
-      address[] memory vaults = kerosineManager.getVaults();
+      address[] memory vaults = vaultManager.getVaults();
      uint numberOfVaults = vaults.length;
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[i]);
        tvl += vault.asset().balanceOf(address(vault)) 
                * vault.assetPrice() * 1e18
                / (10**vault.asset().decimals()) 
                / (10**vault.oracle().decimals());
      }
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }

VaultManagerV2.sol:

contract VaultManagerV2 is IVaultManager, Initializable {
  using EnumerableSet     for EnumerableSet.AddressSet;
  using FixedPointMathLib for uint;
  using SafeTransferLib   for ERC20;

  ...

  mapping (uint => EnumerableSet.AddressSet) internal vaults; 
  mapping (uint => EnumerableSet.AddressSet) internal vaultsKerosene;
  EnumerableSet.AddressSet internal exogeneusVaults;
 
 ...

function add(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
    if (!vaults[id].add(vault))            revert VaultAlreadyAdded();
    exogeneusVaults.add(vault);
    emit Added(id, vault);
  }

 ...

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

Assessed type

Context