Bug fix: kickWithDeposit likely push LUP below HTP affecting healthy loans
#894
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,15 +69,11 @@ library KickerActions { | |
|
|
||
| /// @dev Struct used for `kickWithDeposit` function local vars. | ||
| struct KickWithDepositLocalVars { | ||
| uint256 bucketDeposit; // [WAD] amount of quote tokens in bucket | ||
| uint256 bucketPrice; // [WAD] bucket price | ||
| uint256 currentLup; // [WAD] current LUP in pool | ||
| uint256 entitledAmount; // [WAD] amount that lender is entitled to remove at specified index | ||
| uint256 lenderLP; // [WAD] LP of lender in bucket | ||
| } | ||
|
|
||
| /**************/ | ||
|
|
@@ -140,6 +136,7 @@ library KickerActions { | |
| * @dev - decrement `lender.lps` accumulator | ||
| * @dev - decrement `bucket.lps` accumulator | ||
| * @dev === Reverts on === | ||
| * @dev bucket price bellow current pool `LUP` `PriceBelowLUP()` | ||
| * @dev insufficient deposit to kick auction `InsufficientLiquidity()` | ||
| * @dev no `LP` redeemed to kick auction `InsufficientLP()` | ||
| * @dev === Emit events === | ||
|
|
@@ -157,35 +154,35 @@ library KickerActions { | |
| ) external returns ( | ||
| KickResult memory kickResult_ | ||
| ) { | ||
| KickWithDepositLocalVars memory vars; | ||
|
|
||
| vars.bucketPrice = _priceAt(index_); | ||
| vars.currentLup = Deposits.getLup(deposits_, poolState_.debt); | ||
|
|
||
| // revert if the bucket price is below LUP | ||
| if (vars.bucketPrice < vars.currentLup) revert PriceBelowLUP(); | ||
|
|
||
| Bucket storage bucket = buckets_[index_]; | ||
| Lender storage lender = bucket.lenders[msg.sender]; | ||
|
|
||
| vars.lenderLP = bucket.bankruptcyTime < lender.depositTime ? lender.lps : 0; | ||
| vars.bucketDeposit = Deposits.valueAt(deposits_, index_); | ||
|
|
||
| // calculate amount lender is entitled in current bucket (based on lender LP in bucket) | ||
| vars.entitledAmount = Buckets.lpToQuoteTokens( | ||
| bucket.collateral, | ||
| bucket.lps, | ||
| vars.bucketDeposit, | ||
| vars.lenderLP, | ||
| vars.bucketPrice, | ||
| Math.Rounding.Down | ||
| ); | ||
|
|
||
| // cap the amount entitled at bucket deposit | ||
| if (vars.entitledAmount > vars.bucketDeposit) vars.entitledAmount = vars.bucketDeposit; | ||
|
|
||
| // revert if no entitled amount | ||
| if (vars.entitledAmount == 0) revert InsufficientLiquidity(); | ||
|
|
||
| // kick top borrower | ||
| kickResult_ = _kick( | ||
|
|
@@ -195,81 +192,10 @@ library KickerActions { | |
| poolState_, | ||
| Loans.getMax(loans_).borrower, | ||
| limitIndex_, | ||
|
There was a problem hiding this comment. Quite happy to eliminate all this logic. |
||
| vars.entitledAmount | ||
| ); | ||
| // set LUP to current pool value | ||
| kickResult_.lup = vars.currentLup; | ||
|
There was a problem hiding this comment. Technically, this won't be correct, as the pool's debt will increase slightly. I believe to be totally correct we should recompute the LUP here including the kick penalty interest. There was a problem hiding this comment. makes sense, though should be done for regular kick too, will add this with a commit There was a problem hiding this comment. done in |
||
| } | ||
|
|
||
| /*************************/ | ||
|
|
@@ -381,8 +307,11 @@ library KickerActions { | |
| kickResult_.debtPreAction = borrower.t0Debt; | ||
| kickResult_.collateralPreAction = borrower.collateral; | ||
| kickResult_.t0KickedDebt = kickResult_.debtPreAction ; | ||
|
|
||
| // add amount to remove to pool debt in order to calculate proposed LUP | ||
| // for regular kick this is the currrent LUP in pool | ||
| // for provisional kick this just simulates LUP movement and needs to be reset to current LUP in pool | ||
| kickResult_.lup = Deposits.getLup(deposits_, poolState_.debt + additionalDebt_); | ||
|
|
||
| KickLocalVars memory vars; | ||
| vars.borrowerDebt = Maths.wmul(kickResult_.t0KickedDebt, poolState_.inflator); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kickWithDepositcould be called in the same block as bucket bankruptcy. It would uselender.lpseven though the bucket is bankrupt. Recommend adding a revert like we have inmoveQuoteToken->There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if bucket bankruptcy happens in same block but after kick with deposit is called then
vars.lenderLPwill be 0 making entitled amount to be 0 and tx to revert withInsufficientLiquidityThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking before, what if a bucket bankruptcy occurs in the same block but before
kickWithDepositis called.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think block when kick happens is the constraint here but the block / time of last deposit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was misinterpretting the logic...
if bucket.bankruptcyTime < lender.depositTime ? lender.lps : 0;-> if the bucket was bankrupted in the same block this would be 0 as it should be.All good : )