[H-01] Free DYAD can be existed without collateral because of partial liquidation edge case

[Al-Qa-qa]

This issue introduced when mitigation issue H-02 from code4rena contest

Description

There is support now to make partial liquidation, but the problem is that the liquidator can take all undercollateralized position collateral without burning all DYAD minted by that position.

Since there is a 20% bonus if the position goes below 120% the value is capped to send all collaterals in the vault to the liquidator. However, since there is partial liquidation, the liquidator can escape burning all position DYAD and will still get all the collaterals.

VaultManagerV3.sol#L199

          uint cappedValue = valueToMove > value ? value : valueToMove;

This will cause a critical situation, where it will leave a DYAD with actually zero collaterals (CR = 0). This will affect the coin stability as the coin should have enough collateral in order to retain stability, and in this situation, it will lead to inflation.

The problem will not stop there, as the team may do the liquidation himself and sacrifice the DYAD he will burn in order to retain coin stability. But the problem is that this position will be unliquidatable, and the tx will always revert as the totalValue = 0 in this case (all collaterals have been moved to the previous liquidator), and we divide by totalValue to get the share value.

VaultManagerV3.sol#L196

      uint totalValue  = getTotalValue(id);
      ...

      uint share       = value.divWadDown(totalValue);

So this will end up having free DYAD in the market without any collaterals, and no one can burn it except the owner itself. which will affect the Stability of the coin.

Proof of Concept

Let's say we have only one vault (ETH) || ETH price is 3000$ || collateral is 1.1 ETH || DYAD minted is 3000 || i.e. CR = 110%

VaultManagerV3.sol#L195-L205

      uint totalValue  = getTotalValue(id);
      uint reward_rate = amount.divWadDown(debt).mulWadDown(LIQUIDATION_REWARD);

      uint numberOfVaults = vaults[id].length();
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[id].at(i));
        if (vaultLicenser.isLicensed(address(vault))) {
          uint value       = vault.getUsdValue(id);
          uint share       = value.divWadDown(totalValue);
          uint amountShare = share.mulWadDown(amount);
          uint valueToMove = amountShare + amountShare.mulWadDown(reward_rate);
          uint cappedValue = valueToMove > value ? value : valueToMove;
          uint asset = cappedValue 
                         * (10**(vault.oracle().decimals() + vault.asset().decimals())) 
                         / vault.assetPrice() 
                         / 1e18;

          vault.move(id, to, asset);

Happy Path: The liquidator passed amount = 3000 (total DYAD minted by that bad position)

• reward_rate = (3000 / 3000) * 20% = 20%
• totalValue = 3300
• value = 1.1 * 3000 = 3300
• share = value / totalValue = 1
• amountShare = 1 * amount = 3000
• valueToMove = amonutshare + amountShare reward_rate = 3000 + 3000 20% = 3600
• cappedValue = 3300 (3600 > 3300)
• assets = 1.1 ETH
• Move collaterals...

This is how the liquidate should work, Let's see what will happen if the liquidator makes a partial liquidation.

Issue :The liquidator passed amount = 2800 (and the total DYAD minted is 3000)

• reward_rate = (2800 / 3000) * 20% = 18.66%
• totalValue = 3300
• value = 1.1 * 3000 = 3300
• share = value / totalValue = 1
• amountShare = 1 amount = 1 2800 = 2800
• valueToMove = amonutshare + amountShare reward_rate = 2800 + 2800 18.66% = 3322.48
• cappedValue = 3300 (3322.48 > 3300)
• assets = 1.1 ETH
• Move collaterals...

The liquidator takes all position collaterals and escaped burning 200 DYAD. This will make free 200 DYAD in the market without any collaterals, and the position will be unliquidatable, as liquidate() function will revert if it gets fired again because of division by zero.

Recommended Mitigation

If there is no USD in the position just skip the rest of the execution of the function. so if this scenario occurs, anyone can burn his DYAD, but he will gain nothing.

VaultManagerV3.sol#L186

  function liquidate( ... ) ... {
      ...

      uint totalValue  = getTotalValue(id);
+    if (totalValue == 0) return;
      ...
      for (uint i = 0; i < numberOfVaults; i++) { ...  }

      emit Liquidate(id, msg.sender, to);
  }

[Al-Qa-qa]

The mitigation will not prevent the occurrence of the issue, but it will allow developers (or anyone) to burn the free DYAD.

We cannot force full liquidation if CR lies below 120% as this will make the main issue (Big positions) unable to get liquidated by normal people.

But in case of this thing occurred. and a liquidator made a partial liquidation, and there are some DYAD without any collateral (let's say 200 as the example). anyone can call liquidate again and the function will get executed successfully. as it will not revert to divide by 0 in this case.

I found that this mitigation is the simplest one, but it will force Developers to pay for that DYAD and burn it themselves. As I found that making a check to force full liquidation violates the main issue.

[shafu0x]

fix implemented here: https://github.com/DyadStablecoin/contracts/pull/53

[Al-Qa-qa]

Regarding the Mitigation process.

There is another mitigation which may be better, as it will not lead the protocol devs to burn DYAD themselves.

Since the problem requires two edge cases (CR < 120% and amount != total), we can check for that edge case and do our logic.

The idea will to give the liquidator less bonus in order to preserve, some collaterals in the position, for the DYAD that will be existed. Although it will be not efficient for the liquidator, as its bonus will decrease, it will prevent any existing of DYAD without collaterals.

The idea is simple, We will use the old liquidation logic which was used before, if this edge case occurs.

VaultManagerV3::liquidate()

function liquidate( ... )  ... {
+     uint cr = collatRatio(id);
if (collatRatio(id) >= MIN_COLLAT_RATIO) revert CrTooHigh();
...

• if (cr < 1.2e18 && debt != amount) {
• uint cappedCr = cr < 1e18 ? 1e18 : cr;
• uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(0.2e18);
• uint liquidationAssetShare = (liquidationEquityShare + 1e18).divWadDown(cappedCr);
• uint256 allAsset = value.mulWadUp(liquidationAssetShare);
• asset = allAsset.mulWadDown(amount).divWadDown(debt);
• } vault.move(id, to, asset); ... }

The old implementation will always preserve some collaterals in the position, as it gives the liquidator a percentage of the collaterals in the Vault itself. and this percentage will always be < 100% unless CR falls below or equal to 100%.

Some Examples:

We have only one vault ETH || ETH = 3000$ || DYAD Minted = 3000.

totalAssets	CR	amount	Assets moved in ETH	Assets moved in USD	bonus gained
1.4 ETH	1.4	3000	1.2 ETH	3600$	(3600 - 3000) / 3000 = 20%
1.4 ETH	1.4	1500	0.55 ETH	1650$	(1650 - 1500) / 1500 = 10% (M-02)
1.1 ETH	1.1	3000	1.1 ETH	3300$	(3300 - 3000) / 3000 = 10% (all amount)
1.1 ETH	1.1	1500	0.51 ETH	1530$	(1530 - 1500) / 1500 = 2%

The last example express what happens when doing partial liquidate and cr is below 120%

• liquidationEquityShare = (1.1 - 1) * 0.2 = 0.02 = 2%

And this is the actual bonus that will received by the liquidator in that case.

Now lets see that position (4th example) in that case.

• Left DYAD => 3000 - 1500 = 1500 DYAD

• Collaterals = 1.1ETH - 0.51ETH = 0.59 ETH

• Collateral in USD = 0.59 * 3000 = 1770$

• CR = collaterals / DYAD minted = 1770 / 1500 = 1.18 = 118%

• As we can see the CR increased from 1.1 to 1.18, this happens as the position was 1.1 : 1 ration, i.e. 110%, and the liquidator took the USD collaterals 100% + 2%. i.e. 102%.

• So 110% - 102% = 8% will get increased to the original position.

• So the new CR is 110% + 8% = 118% as stated.

Let me know what you think about this mitigation, I will leave the new function totally to be as a reference if you wanted to implement the mitigation.

new liquidate function

```solidity function liquidate( uint id, uint to, uint amount ) external isValidDNft(id) isValidDNft(to) { uint cr = collatRatio(id); if (cr >= MIN_COLLAT_RATIO) revert CrTooHigh(); uint debt = dyad.mintedDyad(id); dyad.burn(id, msg.sender, amount); // changes `debt` and `cr` lastDeposit[to] = block.number; // `move` acts like a deposit uint totalValue = getTotalValue(id); // @audit why the reward_rate is changing via partial Liquidate uint reward_rate = amount .divWadDown(debt) .mulWadDown(LIQUIDATION_REWARD); uint numberOfVaults = vaults[id].length(); for (uint i = 0; i < numberOfVaults; i++) { Vault vault = Vault(vaults[id].at(i)); if (vaultLicenser.isLicensed(address(vault))) { uint value = vault.getUsdValue(id); uint share = value.divWadDown(totalValue); uint amountShare = share.mulWadDown(amount); uint valueToMove = amountShare + amountShare.mulWadDown(reward_rate); uint cappedValue = valueToMove > value ? value : valueToMove; // @audit can this revert if assets is > the assets in the vault. can this case happens?? uint asset = cappedValue * (10**(vault.oracle().decimals() + vault.asset().decimals())) / vault.assetPrice() / 1e18; if (cr < 1.2e18 && debt != amount) { uint cappedCr = cr < 1e18 ? 1e18 : cr; uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(0.2e18); uint liquidationAssetShare = (liquidationEquityShare + 1e18).divWadDown(cappedCr); uint256 allAsset = value.mulWadUp(liquidationAssetShare); asset = allAsset.mulWadDown(amount).divWadDown(debt); } vault.move(id, to, asset); } } emit Liquidate(id, msg.sender, to); } ```

[shafu0x]

that should be the fix: https://github.com/DyadStablecoin/contracts/pull/57/files

[Al-Qa-qa]

There are somethings in the mitigation I want to illustrate.

1. The implementation uses value, which is the USD$ worth of tokens in the vault as assets, and this is wrong. As we should use assets.

            uint256 allAsset = value.mulWadUp(liquidationAssetShare); 

And this is what was implemented in the old Implementation.

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

This means if the collateral worth 2 Dollars, the function will end up trying to transfer double of the collaterals in the position, which will revert.

Although it is a serious problem, I found that I wrote it like that in mitigation by mistake. In the Remix Environment, I used it with assets to test values, but I wrote it wrongly.

-             uint256 allAsset = value.mulWadUp(liquidationAssetShare);
+             uint256 allAsset = vault.id2asset(id).mulWadUp(liquidationAssetShare);

Since it was my mistake, I will not make another issue for it.

---

2. This mitigation undo the mitigation of issue M-02. As we discussed this issue, it is according to the design choice, and you have the freedom to leave it if the design goal is to decrease the bonus in case of partial liquidation.

let me know the final decision about this issue M-02 and if it will get mitigated or not.

---

3. The Mitigation uses 0.2e18 value as a constant value, and 1.2e18.
   • 0.2e18 can be replaced by LIQUIDATION_REWARD
   • 1.2e18 can be replaced by LIQUIDATION_REWARD + 1e18

This is just formating, but I think it will be better.

[shafu0x]

implemented the fixes here: https://github.com/DyadStablecoin/contracts/pull/57/files

[Al-Qa-qa]

The Mitigation Issue has Been Fixed as Recommended.