DYAD - btk's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L95 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L75

Vulnerability details

Impact

Each DYAD stablecoin is backed by at least $1.50 of exogenous collateral. This surplus absorbs the collateral’s volatility, keeping DYAD fully backed in all conditions. Here is the keypoint to understand the Kerosene token:

• Kerosene is a token that lets you mint DYAD against this collateral surplus.
• Kerosene can be deposited in Notes just like other collateral to increase the Note’s DYAD minting capacity.
• Kerosene is thus as valuable as the degree of DYAD’s overcollateralization.
• Kerosene is not additional collateral; it’s a mechanism for allocating the right to mint against existing surplus collateral (C-D) in the system.

The problem lies in the current implementation, which permits users to mint Dyad tokens solely against Kerosene. This undermines the intended design and the above key points. Consequently:

• Dyad stablecoin's stability is compromised, as it's backed by Kerosene instead of exogenous collateral.
• Kerosene value would go down, since it's value is calculated by the difference of TVL and dyadMinted.

Proof of Concept

In the VaultManagerV2 there is two functions to add a vault:

• add() to add regular vaults
• addKerosene() to add Kerosene vaults

The Deploy.V2.s will license the unboundedKerosineVault:

    vaultLicenser.add(address(unboundedKerosineVault));

Consequently, users can include the unboundedKerosineVault along with their regular vaults:

  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();
    emit Added(id, vault);
  }

The call succeeds because the unboundedKerosineVault is licensed during deployment. However, the issue arises as users can not only add it but also mint Dyad solely against Kerosene by merely (a) adding a Kerosene vault (bound or unbounded), (b) depositing some Kerosene to their position, and (c) minting Dyad against Kerosene.

Here is a coded PoC to demonstrate the issue:

  function testUseKerosineAsCollateral() public {
    uint id = mintDNft();
    vaultManager.add(id, address(_unboundedKerosineVault));
    deposit(weth, id, address(wethVault), 1e22);
    vaultManager.mintDyad(id, 100e18, address(this));

    address user = _setupKerosine();
    uint256 userId = dNft.mintNft(user);

    vm.startPrank(user);
    vaultManager.add(userId, address(_unboundedKerosineVault));
    kerosine.approve(address(vaultManager), 1000000e18); // 1 million
    vaultManager.deposit(userId, address(_unboundedKerosineVault), 1000000e18);
    vaultManager.mintDyad(userId, 100e18, address(this));
    vm.stopPrank();

    assertEq(dyad.collatRatio(id));
  }

Test Setup:

• Replace the BaseTest contract with the one provided here
• Incorporate the tests in VaultManagerTest
• Execute: forge test --mt testUseKerosineAsCollateral

Tools Used

Manual review

Recommended Mitigation Steps

Consider introducing a new KeroseneLicenser specifically for Kerosene vaults, while maintaining the Current Licenser for regular vaults. Then, update the add() & addKerosene() & getKeroseneValue() licensing checks accordingly.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Leaving the deposit function accessible to anyone introduces two issues:

• Attackers can grief users and prevent them from withdrawing their funds.
• Attackers can grief users and prevent them from removing a vault.

Proof of Concept

Below is the implementation of the deposit() function:

  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

As long as the id is a valid NFT, the deposit will succeed. An attacker can exploit this design to grief users.

Withdrawals DoS

The withdraw function prevents deposits and withdrawals in the same block to protect against flash loan attacks:

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

The issue is that this flash loan check can be used against users to prevent them from withdrawing or redeeming their collateral. Let's consider this scenario:

• Alice wants to withdraw or redeem a portion of her collateral from the WETH vault.
• Bob is monitoring the mempool; he sees Alice's transaction and quickly frontruns it with a deposit(AliceId, wethVault, 1 wei).
• Alice's transaction will revert here:

if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();

because her idToBlockOfLastDeposit[AliceId] == block.number.

Kerosene whales are incentivized to launch this attack because the kerosene is as valuable as the degree of DYAD’s overcollateralization. Therefore, locking users' funds will prevent the kerosene price from dropping.

Vault removal DoS

Below is the implementation of the remove() function:

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

There is a sanity check to prevent users from removing a vault that still contains assets. The issue is that any user can deposit assets on behalf of another user. An attacker can exploit this vulnerability by:

• Monitoring the mempool for a user attempting to remove a vault.
• Front-running their transaction and depositing a negligible amount (e.g., 1 wei) to that vault.
• The user's transaction will revert due to (Vault(vault).id2asset(id) > 0).

Here is a coded PoC to demonstrate the issue:

  function testGriefUsers() public {
    uint id = mintDNft();
    vaultManager.add(id, address(wethVault));

    address attacker = makeAddr("Attacker");

    weth.mint(attacker, 1 wei);

    vm.startPrank(attacker);
    weth.approve(address(vaultManager), 1 wei);
    vaultManager.deposit(id, address(wethVault), 1 wei);
    vm.stopPrank();

    vm.expectRevert(); // VaultHasAssets
    vaultManager.remove(id, address(wethVault));
  }

Tools Used

Manual reveiw

Recommended Mitigation Steps

To fix these two issues, consider allowing only DNft owners or their approved operators to call the deposit function:

  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
    address idOwner = dNft.ownerOf(id);
    require(msg.sender == idOwner || dNft.isApprovedForAll(idOwner, msg.sender));
    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Users Kerosing will get permanently locked in the UnboundedKerosineVault.

Proof of Concept

Currently, there is only one exit point for users which is the withdraw function:

  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    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() 
                  * 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(); 
  }

withdraw() will get the value of the amount as follows:

    uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() 
                  / 10**_vault.asset().decimals();

The issue is that this will not work with UnboundedKerosineVault and the call will simply revert because the vault it have no oracle and the code will attampt to call a non-existent variable, see here. Thus, all funds deposited in UnboundedKerosineVault will get permanently locked.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a withdrawKerosine() function that does not call a non-existent variable.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

During lunchtime, the total supply of dyad tokens will exceed the total value locked (TVL) in all vaults. Consequently, an underflow will occur, leading to a temporary locking of funds for users of UnboundedKerosineVault.

Proof of Concept

The function assetPrice() in UnboundedKerosineVault calculates the price of Kerosene as follows:

  function assetPrice() 
    public 
    view 
    override
    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)) 
                * vault.assetPrice() * 1e18
                / (10**vault.asset().decimals()) 
                / (10**vault.oracle().decimals());
      }
      // @audit-issue underflow
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }

The issue arises from the calculation of the numerator, where the total supply of dyad tokens is subtracted from the TVL. Currently, dyad stablecoin is already deployed with 625967.4e18 tokens minted source.

During lunchtime, users depositing Kerosene will be unable to withdraw it, triggering a DoS in the withdraw() function as it calls vault.assetPrice():

uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() 
                  / 10**_vault.asset().decimals();

which will revert due to an underflow here:

//               = 0 - 625967400000000000000000
uint numerator   = tvl - dyad.totalSupply();

Tools Used

Manual review

Recommended Mitigation Steps

There isn't a direct fix for this issue since dyad is already deployed. However, to prevent users' funds from getting locked, consider temporarily disabling the UnboundedKerosineVault. You can re-enable it once the TVL exceeds the total supply of DYAD tokens. Also, consider updating the assetPrice() function to:

  function assetPrice() 
    public 
    view 
    override
    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)) 
                * vault.assetPrice() * 1e18
                / (10**vault.asset().decimals()) 
                / (10**vault.oracle().decimals());
      }
      if (tvl <= dyad.totalSupply()) return 0;
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

The current deployment script overlooks licensing the unboundedKerosineVault in the keroseneManager, preventing users from executing the addKerosene() function.

This omission poses a significant issue: if the owner licenses the unboundedKerosineVault after deployment, it introduce a critical problem. This action leads to an incorrect inflation of the Kerosene price, as the Kerosene vault's total value locked (TVL) is erroneously added to all exogenous collateral in the protocol.

Proof of Concept

Presently, there exist two licensors:

• Licenser, facilitating regular vaults like weth.
• KerosineManager, managing Kerosene vaults such as unbounded vaults.

Users employ the addKerosene() function to add a Kerosene vault:

  function addKerosene(...) external {
    if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
    if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
  }

Each Kerosene vault necessitates licensing in the keroseneManager. However, the deployment script fails to license the unboundedKerosineVault, necessitating manual intervention by the owner:

  function add(
    address vault
  ) 
    external 
      onlyOwner
  {
    if (vaults.length() >= MAX_VAULTS) revert TooManyVaults();
    if (!vaults.add(vault))            revert VaultAlreadyAdded();
  }

Yet, licensing the unboundedKerosineVault in the keroseneManager leads to a critical flaw. Kerosene's calculation involves:

• X: the value of a single Kerosene token deposited in a Note as backing for DYAD
• C: the total USD value of all exogenous collateral in the protocol (TVL). Critically, this total does not include Kerosene itself
• D: the total supply of DYAD stablecoins
• K: the total supply of Kerosene tokens

Here's how it's implemented in the unboundedKeroseneVault:

  function assetPrice() 
    public 
    view 
    override
    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)) 
                * vault.assetPrice() * 1e18
                / (10**vault.asset().decimals()) 
                / (10**vault.oracle().decimals());
      }
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }

Initially, it retrieves licensed vaults from the kerosineManager, which should exclusively include exogenous collateral. However, as per Deploy.V2.s.sol, only ethVault and wstEth vaults are licensed:

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

However, as explained above, the owner must license the unboundedKerosineVault in the kerosineManager for users to call addKerosene().

Consequently, the owner must manually license the unboundedKerosineVault in the kerosineManager for addKerosene() to function properly. As a result, assetPrice() incorporates the unboundedKerosineVault's USD value into the total USD value of exogenous collateral, inflating the Kerosene token price allowing users to abuse this to mint more dyad.

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, consider utilizing a single licenser for all vaults (This approach ensures KerosineManager alone calculates the Kerosene price):

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

import {Owned} from "@solmate/src/auth/Owned.sol";

contract Licenser is Owned(msg.sender) {

  mapping (address => bool) public isLicensed; 
  mapping (address => bool) public isKeroseneLicensed; 

  constructor() {}

  function add            (address vault) external onlyOwner { isLicensed[vault] = true; }
  function remove         (address vault) external onlyOwner { isLicensed[vault] = false; }
  function addKerosene    (address vault) external onlyOwner { isKeroseneLicensed[vault] = true; }
  function removeKerosene (address vault) external onlyOwner { isKeroseneLicensed[vault] = false; }
}

(A) You can then remove the keroseneManager from the VaultManagerV2, as it becomes useless. (B) Update the addKerosene() and getKeroseneValue() functions to call Licenser.isKeroseneLicensed directly.

Assessed type

Other

if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();