c4-bot-4 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L65 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L42-L83

Vulnerability details

Impact

An attacker can exploit one of the old vaults to mint DYAD, which will significantly increase the DYAD supply and subsequently reduce the price of Kerosine. The main consequence is the potential liquidation of healthy loans in new vaults.

Proof of Concept

During the deployment of the Vault manager v2, new empty vaults will be deployed, while the old ones will still be functional to allow depositors to migrate their liquidity.

    VaultManagerV2 vaultManager = new VaultManagerV2(
      DNft(MAINNET_DNFT),
      Dyad(MAINNET_DYAD),
      vaultLicenser
    );

    // weth vault
    Vault ethVault = new Vault(
      vaultManager,
      ERC20        (MAINNET_WETH),
      IAggregatorV3(MAINNET_WETH_ORACLE)
    );

    // wsteth vault
    VaultWstEth wstEth = new VaultWstEth(
      vaultManager, 
      ERC20        (MAINNET_WSTETH), 
      IAggregatorV3(MAINNET_CHAINLINK_STETH)
    );

The newly added Kerosine vault allows depositors to increase the available DYAD amounts for minting based on the current surplus across all vaults. The Kerosine token price is calculated using the formula: X = C - D / K

C - vaults TVL
D - DYAD total supply

K - Kerosine total supply

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 existence of two active vault managers and the introduced Kerosine vault create an opportunity for Kerosine price manipulation. An attacker can take a flash loan (good thing the old vault manager doesn't have protection) to deposit assets with old vault manager, mint a large amount of DYAD tokens and drop Kerosine price down, because TVL in the numerator only calculated for the new vaults. This will cause some healthy positions to fall below 150% collateral ratio and be liquidated by the attacker.

Tools Used

Manual review

Recommended Mitigation Steps

The safest solution would be to deploy Kerosine vaults after the entire liquidity has been migrated from the old vaults and the old vault manager's license has been revoked. Additionally, it would be beneficial to integrate the old vaults into the Kerosene manager contract to ensure that the Total Value Locked (TVL) is calculated with their liquidity included.

  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++) {

However, it's important to note that this approach may not completely safeguard the vault from price manipulation, as long as the old vault manager remains active, the potential for this type of attack still exists.

Assessed type

Other

c4-pre-sort commented 5 months ago

JustDravee marked the issue as duplicate of #308

c4-pre-sort commented 5 months ago

JustDravee marked the issue as sufficient quality report

c4-judge commented 5 months ago

koolexcrypto marked the issue as not a duplicate

c4-judge commented 5 months ago

koolexcrypto marked the issue as unsatisfactory: Out of scope

k4zanmalay commented 5 months ago

I'm curious why this report was marked as OOS? Is it because it mentions old vault manager? If that's the reason, it is not accurate because the vulnerability lies within the Vault.kerosine.unbounded.sol, and the VaultManager.sol is merely a tool to exploit it.

koolexcrypto commented 5 months ago

The main consequence is the potential liquidation of healthy loans in new vaults.

There will be no loans on the new vaults that has Kerosine. This is because Kerosine will be emitted over a very long time and you can't it get it without providing liquidity, so the attacker has to do this. I don't believe there will be a significant amount of Kerosine emitted till the migration finishes.