Al-Qa-qa / dyad-private-audit

4 stars 1 forks source link

[M-02] Partial Liquidation Bonus is set wrongly #7

Open Al-Qa-qa opened 3 weeks ago

Al-Qa-qa commented 3 weeks ago

Description

The Liquidation Bonus is 20% of the bad position collaterals. So if the bad position minted DYAD is 1000 DYAD (USD), the liquidator will earn 1200 USD.

After implementing partial liquidation the liquidator should not earn 20% of all bad position but 20% of the amount he liquidated.

So if we say the bad position is 1000 DYAD (USD), and the liquidator wants partially half liquidate that position (50%), his earning should be: $= 1000 / 2 + (1000 0.2) / 2 = 500 + 100 = 600$$ $= 500 + 500 0.2 = 600$$

So We can calculate the liquidator value by taking the full 20% from the amount he will pay or taking a percentage from the liquidation bonus, relative to the percentage of liquidation to the full position.

The problem is that the implementation performs both of them. where liquidate() function multiplies the partial amount to get liquidate with a partial liquidation rate, which makes the process incorrect.

VaultManagerV3.sol#L198

@>    uint reward_rate = amount.divWadDown(debt).mulWadDown(LIQUIDATION_REWARD);
          ...
          uint amountShare = share.mulWadDown(amount);
@>        uint valueToMove = amountShare + amountShare.mulWadDown(reward_rate);
                                           ^^^^^^^^^^^

Proof of Concept

let's say we have a position with 1000 DYAD, and I will perform partial liquidation by liquidating only 500 DYAD (50%). We will assume that the CR >= 120% and < 150% to ensure no cutoff to the reward will done, and the share = 1.

The value is 550$ not 600$ so it is incorrect, as it should be 600$.

This occurred because we are taking 10% (liquidation bonus / 2) from the amount itself (total dept / 2). which is like dividing by 4 instead of dividing by 2 in this example.

Recommended Mitigation.

Either doing the partial to the bonus or the total amount. The following mitigation is done by reducing the amount.

-         uint valueToMove = amountShare + amountShare.mulWadDown(reward_rate);
+         uint valueToMove = amountShare + amountShare.mulWadDown(LIQUIDATION_REWARD);

And this is if we will do it to the liquidation bonus.

-         uint valueToMove = amountShare + amountShare.mulWadDown(reward_rate);
+         uint valueToMove = amountShare + debt.mulWadDown(reward_rate);

But I prefer the first one as it will not encounter rounding issues.

shafu0x commented 2 weeks ago

fixed here: https://github.com/DyadStablecoin/contracts/pull/54

Al-Qa-qa commented 2 weeks ago

Issue has Been Fixed as Recommended.