fix bug in _calculateSurplusAndCap() for debt redistribution case #674
Conversation
| // now CDP owner should have zero surplus to claim | ||
| cappedColPortion = _totalColToSend; | ||
| } else { | ||
| // for partial liquidation, new ICR would deteriorate | ||
| // since we give more incentive (103%) than current _ICR allowed | ||
| _incentiveColl = (_totalDebtToBurn * LICR) / _price; | ||
| } | ||
| } | ||
| cappedColPortion = collateral.getSharesByPooledEth(_incentiveColl); | ||
| if (cappedColPortion == 0) { | ||
| cappedColPortion = collateral.getSharesByPooledEth(_incentiveColl); | ||
| } |
There was a problem hiding this comment.
Can you ELI5 why this works in this way?
I don't understand why cap would be 0 and if it's 0 then we use the index for it
There was a problem hiding this comment.
@rayeaster I'd second the request here, could use more context
Can you describe how the error is fixed in this case when it previously wasnt in an example?
There was a problem hiding this comment.
@GalloDaSballo @dapp-whisperer When ICR is less than LICR and it is a full liquidation, the _incentiveColl is calculated by getPooledEthByShares(_totalColToSend).
In previous impl, the cappedColPortion is always calculated as getSharesByPooledEth(_incentiveColl), so it essentially equivalent to getSharesByPooledEth(getPooledEthByShares(_totalColToSend)) which might differ some wei from the expected result of _totalColToSend due to rounding error. In Alex's Medusa run, the CDP owner got 1 wei surplus (which shouldn't in theory) to claim
Now with this fix, we just explicitly set cappedColPortion=_totalColToSend for the scenario (full liquidation with ICR < LICR)
|
Mostly trying to understand if:
It may be worth getting more eyes on this specific change and this flow |
|
This merge will prob have conflicts since this doesn't use |
There was a problem hiding this comment.
The code for surplus will benefit by a refactoring which I can apply after we merge down the rest of the code
The logic is basically a 4x4
-> Underwater or Not
-> Full or Partial Liq
If not underwater then math is straightforward
If underwater:
- Assumption 1 Partial doesn't create Bad debt since it leaves some CDP left -> Need to think about this one further
- Assumption 2 Full Liquidation is capped (less coll than debt), so we basically sell discounted coll per debt, which is why we do the extra math back from shares to Value
This causes some of the additional checks to be skippable in different scenarios (gas savings)
The logic is sound, imo can be clarified, and I have a draft refactoring I can send once we merge down this + any conflict
Lastly @dapp-whisperer the naming here doesn't have shares not even in the new PR, worth figuring this one out before doing any code changes
|
great, will merge it down and track this as an issue |
The issue is found during Medusa invariant run and failure-analysis https://gist.github.com/GalloDaSballo/68046d8b089c4d29af40f31d197cc53a?permalink_comment_id=4714393#gistcomment-4714393
When a CDP is fully liquidated with some bad debt distributed (in recovery mode), there should be zero collateral as surplus for CDP owner to claim.
In previous implementation depending on Solidity multiplication and division, it might end with some rounding error that CDP ower got
1 weicollateral as surplus.Now the issue is fixed by this PR