DYAD - forgebyola's results

Lines of code

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

Vulnerability details

Impact

By providing empty deposits to ids of other users, legit users of the protocol can get their withdrawal and redeem in the VaultManagerV2 reverted by a malicious user. Apart from the grief this would cause to users and the protocol, users may also be external protocols or smart contract systems which rely on smooth functionality of the dyad system. Note that the malicious user can DOS withdrawal and redeem for every single user available and for every block. The user only has to worry about gas fees. This would be especially easier in the early adoption of the protocol with relatively less users.

Proof of Concept

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

Due to lack of access control on the VaultManager.deposit function, users can make deposits into any DyadNFT for any vault. This seems to be a system design that allows users to make payments for other users.

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);
  }

Additionally, the block.number at that deposit is set to idToBlockOfLastDeposit[id] for that id. This is to prevent users from withdrawing within the same block as their last deposit, also this check is indirectly enforced during redeem as withdrawal happens during the redeem process.

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

function redeemDyad(
    uint    id,
    address vault,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
    returns (uint) { 
      ...............
     withdraw(id, vault, asset, to);

This is where the malicious user can grief legit users The malicious user can frontrun any withdrawal or redeem in the mempool with a 0 amount deposit into the id of the user, causing the lastdeposit for that id to be reset to that block.number. This would subsequently cause the withdrawal or redeem for the unsuspecting user to revert due to the check. The malicious user can do this for the next block and every single block as long as they intend. Apart from griefing,the bad actor may have other legitimate reasons for doing this and create more attack paths. For example, if the legit user is an external protocol, the bad actor can prevent their withdrawal in order to build an attack on that protocol.

Tools Used

Manual review

Recommended Mitigation Steps

1. A mitigation step would be to dissalow 0 amount deposits or set a minimum deposit such that the bad actor is discouraged due to financial losses.

function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
+  require(amount > 0 /** || amount > minDeposit*/);
    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/VaultManagerV2.sol#L150

Vulnerability details

Impact

Users of the VaultManagerV2 can add both non kerosene Vaults and kerosene Vaults to their DNfts and make deposits into these vaults. Kerosene vaults may be unbounded where users can freely deposit and withdraw kerosene, while deposits in bounded kerosene Vaults cannot be withdrawn. When users make deposits into unbounded Kerosene Vaults they may not be able to withdraw their assets from these vaults. This may cause loss of funds for users of the system.

Proof of Concept

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L150 Users can add make deposits into any vault added to their dnft. Consider a scenario where a user has added an unbounded non-kerosene vault to their id and makes a deposit of 100e18 kerosene to that vault. Now the user wants to withdraw let's say 80e18 assets from that vault. Note that the only way for users to withdraw is through the VaultManagerV2 contract. When the user makes the attempt, since the user does not have a non kerosene vault attached to their id or even if they have, let us assume they do not have any assets in their non-kerosene vaults. Due to the check on L150, the amount to be withdrawn which is 80e18 is substracted from the non kerosene value of the user and reverts if it is less than the dyadMinted for that user. Since the user does not have any value in their non-kerosene vault, their transaction reverts.

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(); 
  }

Essentially, users are expected to have assets in their non-kerosene vaults which is more than the assets in their unbounded kerosene vault in order to withdraw their unbounded kerosene assets. This breaks core protocol functionality as users should be able to withdraw their unbounded kerosene vault asset at any time. Especially considering that users cannot mint Dyad based on their kerosene vault assets, this check should not apply to users when withdrawing from unbounded kerosene vaults

Tools Used

Manual review

Recommended Mitigation Steps

1. Only check non kerosene value against withdrawal amount if the vault is a non-kerosene vault.

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();
+ if(_vault.isNonKeroseneVault) {
      if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat(); 
}
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Unsuspecting users of the protocol who rely on deposited assets in the kerosene vaults to avoid liquidation can get maliciously liquidated by a user with significant portions of the deposited assets in the kerosene vaults. Essentially, users of the protocol would lose funds in this scenario.

Proof of Concept

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

The asset price of kerosene is determined by the tvl of kerosene assets in kerosene vaults (total value deposited into kerosene vaults) and dyad totalsupply which is controlled by minting or burning dyad tokens. assetprice of kerosene = tvl - dyadTotalSupply/denominator

  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;

The denominator is determined by the kerosene totalSupply (1billion*WAD) and the value owned by the mainnet owner. For most scenarios this value can be expected to be constant unless the mainnet owner makes large moves. Therefore, this asset price is mostly determined by how much is deposited in kerosene vaults and how much dyad is in circulation. There are 2 ways to drop the assetprice

1. Withdraw previously deposited kerosene tokens in large amounts.
2. Mint large amounts of dyad against collateral to increase dyad totalsupply

To increase the assetprice, the opposite can be done.

Now why would a user who has enough value to shift asset price make such moves. This is because users rely on this asset price to maintain their collatRatio against liquidation.

function liquidate(
    uint id,
    uint to
  ) 
    external 
      isValidDNft(id)
      isValidDNft(to)
    {
      uint cr = collatRatio(id);
 @>     if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
     .................................

 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 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;
  }

Users are incentivized to deposit assets into kerosene vaults especially bounded kerosene vaults (since this doubles their assets) to hedge against liquidation. If the total value of that user goes below a certain range (collatRatio), they would get liquidated. If users are liquidated by another user, this user gets to burn dyad tokens for legit deposited assets of the liquidatee. Therefore, this malicious user who controls 40% of the total assets in kerosene vaults (for example), can maliciously withdraw all their assets in one transaction, causing the asset price of kerosene to tank. Since the total usd value of users who have deposited and minted dyad, and rely on their kerosene assets to maintain their collatRatio is now reduced. This whale can then liquidate unfairly all users whose positions are now liquidatable.

After mass liquidations such as this, the user now has even more assets and can deposit again to raise the price up even more, making massive profits

Tools Used

Manual review

Recommended Mitigation Steps

1. Make use of a weighted assetprice which averages the price of assets in the kerosene vaults over a number of timestamps and returns this weighted price as the asset price of kerosene. This is how twap price works on Uniswap and is the recommended method of mitigating oracle price manipulation vulnerabilities.

Assessed type

Oracle