TVL (Exogenous collateral) backing Dyad can fall below 1:1 while all positions in the protocol remain above `MIN_COLLAERIZATION_RATIO` so cannot be liquidated

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact VaultManagerV2 checks a DNft's exogenous collateral is at least equal to their amount of minted Dyad in both withdraw and mintDyad. However after the Dyad is minted there is nothing to require that that ratio remains at least 1:1. This is because to liquidate a user it is only necessary that their collateralization ratio falls below MIN_COLLATERIZATION_RATIO which can be covered by the users Kerosene holdings.

Furthermore as all the collateral in the protocol will be significantly correlated to the price of Ether, if the price of Ether was to drop a large amount it would very likely mean that multiple users drop below the 1:1 exogenous collateral to dyad minted ratio, potentially resulting in the overall TVL of the protocol falling below the amount of dyad minted. This breaks the main invariant of the protocol and would lead to the stable coin being undercollateralized.

As well as this, UnboundedKeroseneVault::assetPrice relies on TVL being greater than dyad.totalSupply as the following calculation will otherwise cause an underflow revert:

  uint numerator   = tvl - dyad.totalSupply();

Proof of Concept Testing this is currently complicated because of the existing issue of dyad.totalSupply() being non-zero based on dyad minted from the original VaultManager contract.

However the idea is relatively simple:

1. Protocol launches, users deposit collateral & kerosene.
2. User wish to mint dyad at as close to a 1:1 ratio to their collateral as possible to be most capital efficient.
3. The users collateral ratio is covered significantly by their Kerosene holdings.
4. The price of Ether drops 10%.
5. The users collateral ratio may still remain above 150% based on their Kerosene holdings (meaning their position cannot be liquidated)
6. However the tvl of the protocl is now likely less than the total supply of dyad in circulation, breaking the core invariant of the protocol.

Add the following test to v2.t.sol for an example highlighting this:

  function testCollateralRatioBreak() public {
    licenseVaultManager();
    // Dummy vault returns eth price to be $3000 until changed
    (Vault dummyWeth, DummyOracle dummyOracle) = addDummyWethVault();
    uint keroseneAmount = 400_000 ether;

    // Set up alice, bob and chad
    address alice = makeAddr("alice");
    uint aliceTokenId = sendNote(alice);
    getWETH(alice, 5 ether);
    sendKerosene(alice, keroseneAmount); 

    address bob = makeAddr("bob");
    uint bobTokenId = sendNote(bob);
    getWETH(bob, 5 ether);
    sendKerosene(bob, keroseneAmount);

    address chad = makeAddr("chad");
    uint chadTokenId = sendNote(chad);
    getWETH(chad, 5 ether);
    sendKerosene(chad, keroseneAmount);

    address[] memory depositors = new address[](3);
    depositors[0] = alice;
    depositors[1] = bob;
    depositors[2] = chad;

    uint[] memory tokenIds = new uint[](3);
    tokenIds[0] = aliceTokenId;
    tokenIds[1] = bobTokenId;
    tokenIds[2] = chadTokenId;

    // Add TVL to match value of currently minted Dyad on mainnet (~$625k)
    boostTVL(address(dummyWeth), 209 ether);
    // Add dummyWeth vault to kerosene manager
    vm.prank(MAINNET_OWNER);
    contracts.kerosineManager.add(address(dummyWeth));

    // The three users deposit their ETH & Kerosene
    for (uint i; i < depositors.length; ++i) {
      address user = depositors[i];
      uint tokenId = tokenIds[i];

      vm.startPrank(user);
      contracts.vaultManager.add(tokenId, address(dummyWeth));
      contracts.vaultManager.addKerosene(tokenId, address(contracts.unboundedKerosineVault));

      WETH(payable(MAINNET_WETH)).approve(address(contracts.vaultManager), 5 ether);
      contracts.vaultManager.deposit(tokenId, address(dummyWeth), 5 ether);

      Kerosine(MAINNET_KEROSENE).approve(address(contracts.vaultManager), keroseneAmount);
      contracts.vaultManager.deposit(tokenId, address(contracts.unboundedKerosineVault), keroseneAmount);
      contracts.vaultManager.mintDyad(tokenId, 10000 ether, user);

      vm.stopPrank();
    }

    // Lower value of eth (drops 10%) 
    dummyOracle.setAnswer(270000);

    // Attempt to get kerosene value reverts because of tvl - dyad total supply underflow
    contracts.vaultManager.getKeroseneValue(aliceTokenId);
  }

  // Helper functions

  function sendNote(address _to) internal returns(uint) {
    vm.startPrank(MAINNET_OWNER);
    uint tokenId = DNft(MAINNET_DNFT).mintInsiderNft(_to);    
    vm.stopPrank();
    return tokenId;
  }

  function sendKerosene(address _to, uint _amount) internal {
    vm.startPrank(MAINNET_OWNER);
    Kerosine(MAINNET_KEROSENE).transfer(_to, _amount);
    vm.stopPrank();
  }

  function licenseVaultManager() internal {
    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));
  }

  function addDummyWethVault() internal returns(Vault, DummyOracle) {
    // Set up a dummy weth vault to help simulate price movement
    DummyOracle dummyOracle = new DummyOracle();
    Vault dummyWeth = new Vault(contracts.vaultManager,ERC20(MAINNET_WETH), IAggregatorV3(address(dummyOracle))); 
    vm.prank(MAINNET_OWNER);
    contracts.vaultLicenser.add(address(dummyWeth));
    return (dummyWeth, dummyOracle);
  }

  function getWETH(address _to, uint _amount) internal {
    vm.deal(_to, _amount);
    vm.startPrank(_to);
    WETH(payable(MAINNET_WETH)).deposit{value: _amount}();
    vm.stopPrank();
  }

  function boostTVL(address _vault, uint _amount) internal {
    getWETH(_vault, _amount);
  }

The test returns the following error:

[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] testCollateralRatioBreak() (gas: 2881820)

Recommended Mitigation It's likely necessary that VaultManagerV2::liquidate considers a DNft at risk of liquidation if it's collateral ratio is < 150% OR it's exogenous collateral ratio is below 100%. This would protect the Dyad stable coin from being under collateralised as users would be able to liquidate these positions and return the tvl:dyad supply ratio back to a positive one.

Assessed type

Other

[c4-pre-sort]

JustDravee marked the issue as high quality report

[c4-pre-sort]

JustDravee marked the issue as primary issue

[0xMax1]

A users' ability to shield themselves from liquidation with kerosene is considered a feature, not a bug. Even if kerosene's utilization reached 100%, protocol wide overcollat would still be 125%

@shafu0x I suggest we label issue 1027 as sponsor disputed.

[c4-judge]

koolexcrypto marked the issue as duplicate of #308

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto marked the issue as duplicate of #338

[c4-judge]

koolexcrypto marked the issue as satisfactory

[McCoady]

This report outlines more clearly that the issue here is specifically about overall protocol health (maintaining the > 1:1 ratio between collateral and dyad minted), avoiding large scale liquidations / stablecoin depegging.

I believe this report should be considered as the primary issue for this duplicate group, if not a separate issue entirely given that the the root of the issue outlined here is the protocols inability to adequately handle significant downwards ETH price action (all positions are effectively ETH longs). Adding the ability to liquidate positions with a sub 1:1 ratio is the suggested mitigation to keep the protocol healthy rather than the root cause as outlined the in the current primary issue.

[koolexcrypto]

Thank you for the feedback.

After reviewing, I still believe the main issue stands, as it is a bit clearer and easier to understand.