DYAD - Krace's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L230-L248 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L250-L286 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/script/deploy/Deploy.V2.s.sol#L36-L113

Vulnerability details

Impact

The holders can mint DYAD at a collateralization ratio lower than 150% and remain unliquidated because the UsdValue of same vaults may be added to the TotalUsdValue twice.

Proof of Concept

The calculation of collatRatio is based on the TotalUsdValue of DNft id. And the TotalUsdValue is the sum of NonKeroseneValue and KeroseneValue.

  function collatRatio(
    uint id
  )
    public 
    view
    returns (uint) {
      uint _dyad = dyad.mintedDyad(address(this), id);
      if (_dyad == 0) return type(uint).max;
      return getTotalUsdValue(id).divWadDown(_dyad);
  }

  function getTotalUsdValue(
    uint id
  ) 
    public 
    view
    returns (uint) {
      return getNonKeroseneValue(id) + getKeroseneValue(id);
  }

However, according to the Deploy.V2.s.sol, the KerosineManager(which is used to calculate the KeroseneValue) and the vaultLicenser(which is used to calculate the NonKeroseneValue) both own the ethVault and the wstEthVault. This implies that the UsdValue of these two vaults will be added to the TotalUsdValue twice if the owner of DNft adds these two vaults in the vaults and the vaultsKerosene. Consequently, this inflation of the TotalUsdValue leads to an exaggerated collatRatio, surpassing the intended design documentation: they deposit at a 150% minimum collateralization ratio.

//@audit The vaults in VaultLicenser is used here
  function getNonKeroseneValue(
    uint id
  ) 
    public 
    view
    returns (uint) {
      uint totalUsdValue;
      uint numberOfVaults = vaults[id].length(); 
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[id].at(i));
        uint usdValue;
        if (vaultLicenser.isLicensed(address(vault))) {
          usdValue = vault.getUsdValue(id);        
        }
        totalUsdValue += usdValue;
      }
      return totalUsdValue;
  }
//@audit The vaults in KerosineManager is used here
  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;
  }

  function run() public returns (Contracts memory) {
    vm.startBroadcast();  // ----------------------

    Licenser vaultLicenser = new Licenser();

    // Vault Manager needs to be licensed through the Vault Manager Licenser
    VaultManagerV2 vaultManager = new VaultManagerV2(
      DNft(MAINNET_DNFT),
      Dyad(MAINNET_DYAD),
      vaultLicenser
    );

    // weth vault
    Vault ethVault = new Vault(
      vaultManager,
      ERC20        (MAINNET_WETH),
      IAggregatorV3(MAINNET_WETH_ORACLE)
    );

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

    KerosineManager kerosineManager = new KerosineManager();

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

    [...]

    vaultLicenser.add(address(ethVault));
    vaultLicenser.add(address(wstEth));
    vaultLicenser.add(address(unboundedKerosineVault));
    // vaultLicenser.add(address(boundedKerosineVault));

    [...]
  }

Tools Used

Manual Review

Recommended Mitigation Steps

When calculating the collatRatio, avoid accumulating the UsdValue of duplicate vaults.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/VaultManagerV2.sol#L119-L131 https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/VaultManagerV2.sol#L134-L153

Vulnerability details

Impact

A malicious user could deposit zero assets to the victim DNft, updating the idToBlockOfLastDeposit[id] to the current block.number. This action would prevent the owner of id from withdrawing their deposits because that the withdraw function will revert if idToBlockOfLastDeposit[id] is equal to block.number. As long as the malicious user continues to call deposit every block, the victim will be unable to retrieve their deposits.

Proof of Concept

Anyone can deposit any amount of assets into a valid DNft, updating the idToBlockOfLastDeposit[id] to the current block.number.

  /// @inheritdoc IVaultManager
  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
  //@audit anyone could deposit to any valid DNft to update its idToBlockOfLastDeposit
    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

The withdraw function necessitates that idToBlockOfLastDeposit[id] does not equal the current block.number. Hence, a malicious user could deposit zero assets into the victim DNft every block to impede the victim from withdrawing their deposits.

  /// @inheritdoc IVaultManager
  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
  //@ revert if idToBlockOfLastDeposit[id] == block.number
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
    uint dyadMinted = dyad.mintedDyad(address(this), id);
    Vault _vault = Vault(vault);
    uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() 
                  / 10**_vault.asset().decimals();
    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Only allow the owner of DNft to deposit.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/script/deploy/Deploy.V2.s.sol#L48-L65 https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/Vault.kerosine.unbounded.sol#L50-L68

Vulnerability details

Impact

The old WETH vault and wstETH vault managed by VaultManagerV1 contains assets leveraged to create the current Dyad. However, the old vaults are not added to VaultManagerV2, and new vaults are created instead. This results in the TVL becoming zero after migration because there are no assets in the new vaults. However, the total supply of DYAD is not zero, thus breaking the protocol's invariants.

Proof of Concept

The Deploy script of migration will create two new vault to the KerosineManager.

  function run() public returns (Contracts memory) {
    vm.startBroadcast();  // ----------------------

    Licenser vaultLicenser = new Licenser();

    // Vault Manager needs to be licensed through the Vault Manager Licenser
    VaultManagerV2 vaultManager = new VaultManagerV2(
      DNft(MAINNET_DNFT),
      Dyad(MAINNET_DYAD),
      vaultLicenser
    );

    // weth vault
    Vault ethVault = new Vault(
      vaultManager,
      ERC20        (MAINNET_WETH),
      IAggregatorV3(MAINNET_WETH_ORACLE)
    );

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

    KerosineManager kerosineManager = new KerosineManager();

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

And the assetPrice of Unbounded Vault is calculatd based on the vaults in the kerosineManager. However, the old WETH vault and wstETH vault are not added to the VaultManagerV2, and new vaults are created, then the TVL could be zero because there are no assets in the new created vaults. This clearly violates the invariant that the TVL should exceed the total supply of DYAD.

  function assetPrice() 
    public 
    view 
    override
    returns (uint) {
      uint tvl;
      //@audit tvl will be zero because the vaults are new
      //@audit there are no asset in the new vaults
      address[] memory vaults = kerosineManager.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;
  }

POC

Add the test to test/fork/v2.t.sol and run it with forge test --match-test test_assetPrice --fork-url https://eth-mainnet.g.alchemy.com/v2/<API_KEY>.

The call to unboundedKerosineVault.assetPrice() will revert directly because the tvl is zero and the total supply of DYAD is not zero.

diff --git a/test/fork/v2.t.sol b/test/fork/v2.t.sol
index 349412f..d86e451 100644
--- a/test/fork/v2.t.sol
+++ b/test/fork/v2.t.sol
@@ -7,7 +7,6 @@ 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";
-
 contract V2Test is Test, Parameters {

   Contracts contracts;
@@ -52,4 +51,11 @@ contract V2Test is Test, Parameters {
     uint denominator = contracts.kerosineDenominator.denominator();
     assertTrue(denominator < contracts.kerosene.balanceOf(MAINNET_OWNER));
   }
+
+  function test_assetPrice() public {
+
+    uint256 x = contracts.unboundedKerosineVault.assetPrice();
+    console.log(x);
+
+  }
 }

Tools Used

Foundry

Recommended Mitigation Steps

Add the old WETH vault and wstETH vault to the VaultManagerV2 when migrating.

Assessed type

Upgradable