DYAD - Giorgio's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L34 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/script/deploy/Deploy.V2.s.sol#L95-L96 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L67-L78 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L250-L267

Vulnerability details

In order to determine the current collateral a user has, both values of the vaults and vaultsKerosene are added together. The issue we are facing here is that Kerosene vaults can be added to vault, essentially creating collateral that is 100% Kerosene based and doubled in value.

Impact

This will allow for borrowers to take undercollaterized positions.

Proof of Concept

In the deployment script and the video that the dev posted we can see a clear intent of licensing both kerosene vaults.

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

Since now both Kerosene vaults are licensed, this will allow anyone to add them in the vaults mapping.

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

This is will completely break the logic of the protocol since, getTotalUsdValue() will now calculate the value of kerosene + kerosene, allowing any users to not only get their collateral doubled, but also provide kerosene only(endogenous collateral) which is initially supposed to support 1/3 rd of the position maximum.

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

Tools Used

Manual review

Recommended Mitigation Steps

The easiest way to mitigate this issue is to not license the kerosene vaults.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L215

Vulnerability details

In order to liquidate an undercollaterized position the liquidator must pay the borrower's minted DYAD. This means that the liquidator must have at least enough liquidity to cover the borrowed amount. If a whale decides to borrow 50 000 000 DYAD there might not be many people able to cover this much liquidity, hence the position becomes hardly liquidatable.

Impact

Huge positions can make the protocol become insolvent.

Proof of Concept

In the liquidate function we can see that we need to supply the borrowed amount of the liquidatee.

  function liquidate(
    uint id,
    uint to
  ) 

           .  .  .
 @>   dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

But if some actors with immense amount of liquidity decide to deposit and borrow it will become very hard to liquidate them if their position doesn't satisfy the collateral ratio.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a limit to how much a user can borrow, 400 000 USD per NFT sounds reasonable.

  function mintDyad(
    uint    id,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
  {
    uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
++  if(newDyadMinted > 400_000 eth ) revert;
    if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();
    dyad.mint(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); //@note which means can be 150 
    emit MintDyad(id, amount, to);
  }

Assessed type

Other

Lines of code

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

Vulnerability details

Whenever a deposit is made in the VaultManagerV2 the block.number is registered. Whenever a withdrawal is being processed the block.number is being checked and cannot be the same as the the one registered in the deposit(), which means that deposit and withdrawal cannot happen in the same block for a user. The issue at hand is that a user can be denied from withdrawing if a malicious actor frontruns the user's withdrawals and deposits 1 wei of assets in the user's vault.

Impact

Users will experience withdraw denial.

Proof of Concept

The deposit() function will register when deposits are made into a vault.

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

And withdraw()als revert when idToBlockOfLastDeposit[id] == block.number

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

This means that anyone can deposit one wei worth of assets in order to constantly front run the withdrawer whenever he fires a withdrawal transaction.

Attack scenario

Bob wants to attack Eve, he sets up a bot that frontruns any of Eve's withdrawal requests.

Eve withdraws. Bob's bot deposits. Eve transaction reverts. Eve is sad

Tools Used

Recommended Mitigation Steps

In order to mitigate this issue consider adding a minimum deposit, this will make such attacks very costly, undoable.

  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
+ require(amount > minDepositAmount, "deposit too small");

Assessed type

DoS

Lines of code

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

Vulnerability details

The price of $kerosene is determined by the several factors, such as the tvl - borrowed assets and circulating supply of kerosene. The issue here is that if the tvl happens to be lower than the total borrowed amount, minor insolvency, the kerosene price function will revert, locking the whole protocol.

Impact

The protocol will suffer DOS for any calls to withdraw(), mintDyad() and liquidate(). Given that positions sinking the boat are not liquidatable the impact will be severe.

Proof of Concept

The issue at hand stems from the UnboundedKerosineVault::assetPrice.

//UnboundedKerosineVault.sol 
//assetPrice()
@>    uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator(); // e18 
      return numerator * 1e8 / denominator;

Here above we can see that the numerator is calculated by subtracting the total borrowed assets from the tvl.

Since the protocol only allows to borrow assets if the collaterization ratio of the borrower is >= to 150%, tvl - dyad.totalSupply() might appear as always > 0. However this might not be the case if the value the collateral asset crashes abruptly, in this situation the liquidators might no be able to liquidate the positions in time due to the chain being clogged and hardly usable. When the protocol becomes insolvent, the assetPrice() function will revert, causing all the related functions that rely on assetPrice() to also revert, leading to dos. The assetPrice() is called in the

getUsdValue function

  function getUsdValue(
    uint id
  )
    public
    view 
    returns (uint) {
 @>     return id2asset[id] * assetPrice() / 1e8; 
  }

The get getUsdValue() is being called in getKeroseneValue, which is being called in getTotalUsdValue and then getTotalUsdValue is being called in collatRatio. So every function that relies on collatRatio() will revert if the protocol happens to be insolvent. The concerned functions are withdraw(), mintDyad() and liquidate().

Tools Used

Recommended Mitigation Steps

In case the protocol becomes momentarily it is important to keep the assetPrice() from reverting, this will allow the protocol to be able to liquidate insolvent positions. This can be done as such :

+     if(dyad.totalSupply() >= tvl) return 0;
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator(); 
      return numerator * 1e8 / denominator;

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L221-L225

Vulnerability details

During the liquidation process, assets get transferred to the liquidator. But the kerosene assets are not being transferred.

Impact

The liquidator expects to receive kerosene assets, liquidator may not be able to cover the liquidated position. Liquidation incentive decreases. Protocol accumulates bad debt. DYAD peg is lost.

Proof of Concept

To confirm this vulnerability we just have to look at the liquidate function.

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

As we can clearly see, the rewarding process only loops through the vaults mapping, however vaultsKerosene also holds rewards to be moved.

Tools Used

Manual review

Recommended Mitigation Steps

Add a loop to liquidate kerosene vaults as well.

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

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L225

Vulnerability details

When borrowers deposit Kerosene tokens into the KeroseneBoundedVault, the deposited tokens become illiquid, rendering them non-withdrawable. Consequently, the restricted Kerosene tokens doubles in value. Allowing for the borrower to borrow more with the same amount of Kerosene. The issue is when this borrow position is at risk of liquidation due to insufficient collateral, the kerosene liquidated will be bought at two fold the normal price.

Impact

Liquidators earn less rewards, because in the bounded kerosene is just inflated kerosene. Resulting in a decreased liquidation incentive.

Proof of Concept

In order to liquidate a borrower position, the liquidator has to buy the borrower's loan.

dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

The liquidattions process will go through every vault, and transfer the amount amount to the liquidator.

//VaultManagerV2.sol

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

However since the Kerosene in the bounded Kerosene vault is inflated,

//BoundedKerosineVault

  function assetPrice() 
    public 
    view 
    override
    returns (uint) {
      return unboundedKerosineVault.assetPrice() * 2;
  }

Let's have a quick look at the graph below.

https://www.desmos.com/calculator/nklmpzug4g

In the best case scenario, the liquidator is able to earn 10% of liquidated assets as a reward.

Let's consider a postion that has 99 USD worth of wETH and 50 USD worth of bounded Kerosene. This position initially borrowed 100 DYAD and is now being liquidated. In a normal setting the liquidator should be able to receive 15 USD worth of assets as a reward. $99 + 50 = 149$ 10 % 149 = 15 (div up).

But since the bounded kerosene value is inflated, the liquidator will earn less rewards, 12 USD.

$99 + 25 = 124$ 10 % 12

This results in 20 % expected reward decrease for the liquidator which is quite considerable, this assumes an example where Kerosene accounts 1/3 rd of the collateral. But when liquidated position could represent higher amounts of bounded kerosene and thus even greater reward decrease.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue consider normalizing the inflated Kerosene before transferring it to the liquidator.

A simple fix would be to simply move all the bounded Kerosene to the liquidator.

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

Other more precise solution might require more calculation.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L215-L219

Vulnerability details

To initiate a liquidation, the protocol requires the liquidator to repay the outstanding debt (borrowed amount) of the undercollateralized position(>150%). In return, the liquidator receives a bonus up to <10% of the liquidated position collateral. The current issue that the protocol faces is that there is no incentive to liquidate positions that that are at 100% or below.

Impact

Insolvent position don't get liquidated, the protocol accumulates bad debt, the protocol might end up losing its peg.

Proof of Concept

The graph below describes the relation between the liquidation reward to collateral ratio.

https://www.desmos.com/calculator/fchm6grwwt

We can observe that as the collateralization ratio approaches one, the incentive for liquidators diminishes. Once the collateralization ratio reaches exactly one, liquidators would not earn any rewards, and they may even incur losses due to gas transaction costs. While liquidators can potentially earn up to >10% of the liquidated position as a reward, this incentive becomes less attractive when the collateralization ratio nears one. In such cases, considering the gas costs involved and the possibility of earning only a small amount of bounded and illiquid tokens like Kerosene, liquidating positions with a collateralization ratio close to one becomes unappealing for liquidators.

//VMV2.sol
//liquidate()
      uint cappedCr               = cr < 1e18 ? 1e18 : cr; // cappedCr >= 1
      uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD); // cappedCr - 1 * 0.2
      uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr); // ((cappedCr - 1 * 0.2) + 1) / cappedCr 

  . . .
uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare); // collateral *  ((cappedCr - 1 * 0.2) + 1) / cappedCr)

In case an asset value plunges abruptly, which is infrequent but may occur in crypto, and the position becomes undercollateralized before a liquidator has a chance to liquidate it due to chain congestion or other factors, the undercollateralized position may not be liquidated. This will bring bad debt upon the protocol.

Tools Used

desmos

Recommended Mitigation Steps

Consider adding a fixed liquidation reward for when the position cr is below 1.01e18. A fixed amount of kerosene could be sent to the liquidator instead of having the liquidator repay for the position, to at least cover gas costs and incentivize the termination of insolvent positions.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L165-L167

Vulnerability details

When a borrow position's collateral ratio falls below 150%, it becomes subject to liquidation. Liquidation of positions with a cr between [100% and 150%[ will at most earn <10% of the collateral, this will not be enough to cover the mainnet gas costs for liquidation of a small borrow postion of 1 USD, hence the protocol will accumulate bad debt.

Impact

There is no incentive to liquidate small positions when they become undercollaterized. This means that the protocol will accumulate bad debt, if the protocol accumulate too much bad debt it may become insolvent, and the stable coin will lose its peg.

Proof of Concept

If we take a look at the mintDyad

  function mintDyad(
    uint    id,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
  {
    uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
    if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();
    dyad.mint(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
    emit MintDyad(id, amount, to);
  }

We can see that there is not min borrow amount, small undercollaterized can thus be accumulated. Since gas cost can be relatively high on mainnet, these positions will remain unliquidated. They are still technically liquidatable but at a loss that no liquidator will take.

Tools Used

Manual review

Recommended Mitigation Steps

In order to mitigate this issue, consider adding a minimum borrow position, could be 100 usd for example.

Here's how to implement this fix:

function mintDyad(
    uint    id,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
  {

    uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
++  require(newDyadMinted >= 100e18, "Borrow more");
    if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();
    dyad.mint(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
    emit MintDyad(id, amount, to);
  }

Don't forget to modify the redeem function as well.

++ function isAbove100(uint id) returns(bool) {

++   if(dyad.mintedDyad(address(this), id) < 100e18 && dyad.mintedDyad(address(this), id) != 0) return false;
++   return true;

}

  function redeemDyad(
    uint    id,
    address vault, //@note I can chose the vault .. 
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
    returns (uint) { 
      dyad.burn(id, msg.sender, amount);
++    if(!isAbove100(id)) revert;
      Vault _vault = Vault(vault);
      uint asset = amount  
                    * (10**(_vault.oracle().decimals() + _vault.asset().decimals()))  
                    / _vault.assetPrice() 
                    / 1e18;  
      withdraw(id, vault, asset, to);
      emit RedeemDyad(id, vault, amount, to);
      return asset;
  }

  function liquidate(
    uint id,
    uint to
  ) 
    external 
      isValidDNft(id)
      isValidDNft(to)
    {
      uint cr = collatRatio(id);
      if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
      dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));
++    if(!isAbove100(id)) revert;

. . .

  function burnDyad(
    uint id,
    uint amount
  ) 
    external 
      isValidDNft(id)
  {
    dyad.burn(id, msg.sender, amount);
++  if(!isAbove100(id)) revert;
    emit BurnDyad(id, amount, msg.sender);
  }

Assessed type

Other