Bug fix: `kickWithDeposit` likely push `LUP` below `HTP` affecting healthy loans #894

grandizzy · 2023-06-14T15:40:22Z

Description of change

High level

Current approach removes deposits but since it performs a remove-quote-token like action is lacking check to:
- revert if auction debt locked
- revert if new LUP below HTP
Without these changes, after kicking max loan LUP would likely fall below HTP hence drawing many more loans undercolalteralized / kickable, which is not desired.
Introducing these changes renders function unusable as in most cases one of the two reverts would kick in.
New simplified approach of kick with deposit function (renamed to lenderKick):
- does not change deposits, hence doesn't move LUP and no remove-quote-token like check required.
- lender should only prove that they are entitled with enough funds in bucket that, if removed, would draw loan undercollateralized at proposed LUP (hence kickable).
- since deposits are not changed, the new approach require lender to post bond (similar with regular kick) in order to kick the top loan
- it simplifies a lot function logic as it only calculates lender's entitled amount, and performs same _kick procedure as regular kick (with entitled amount as additional debt)
Additionally, the LUP was not recalculated in order to include additional kick penalty debt.

Description of bug or vulnerability and solution

Due to lack of deposit removal check, kickWithDeposit likely push LUP below HTP affecting healthy loans
Fixed by:
- in kickWithDeposit remove logic to remove from deposits
- revert with PriceBelowLUP if bucket price is lower than current LUP - in this case lender should call removeQuoteToken function
- calculate lender's entitled amount in bucket (based on their LP) and caped by bucket deposit size
- revert with InsufficientLiquidity if calculated entitled amount is 0
- call _kick function passing entitled amount as additional debt to be used when proposed LUP is calculated. Same logic applies, if deposit is not enough to render loan undercollateralized then tx reverts with BorrowerOk error
- in _kick function recalculate LUP including debt resulted from kick penalty (for both regular kick and kick with deposit actions) - introduced KickResult.poolDebt struct member, returned and used in Pool contract
- no more RemoveQuoteToken event emitted by kickWithDeposit
Tests updates:
- QT3 invariant updated to reflect that bonds are always guaranteed (previously this invariant failed when using deposits to cover bonds in a pool without enough liquidity)
- test helper _kickWithDeposit changed to receive only the bond amount (that should be transferred from kicker) and to remove RemoveQuoteToken emit check
- updated unit tests

Contract size

Pre Change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  24,384B  (99.21%)
  ERC20Pool                -  23,612B  (96.07%)

Post Change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  24,497B  (99.67%)
  ERC20Pool                -  23,739B  (96.59%)

Gas usage

Pre Change

| kick                                 | 149467          | 223488 | 221980 | 1034805 | 48000   |
| kickWithDeposit                      | 199590          | 277221 | 273367 | 1000575 | 8000    |

Post Change

| kick                                 | 172049          | 246062 | 244559 | 1057386 | 48000   |
| lenderKick                      | 216107          | 293739 | 289884 | 1018335 | 8000    |

Merge develop

mattcushman · 2023-06-14T16:18:38Z

src/libraries/external/KickerActions.sol

-
- if (bucket.bankruptcyTime < lender.depositTime) vars.lenderLP = lender.lps;
+ uint256 lenderLP = bucket.bankruptcyTime < lender.depositTime ? lender.lps : 0;
+ uint256 bucketDeposit = Deposits.valueAt(deposits_, index_);


I think we should add a check that index_ is greater to or equal to the lup here -- otherwise the provisional removal of the deposit would not actually lower the lup. In that case, the lender can just call removeQuoteToken if they want

done at https://github.com/ajna-finance/contracts/pull/894/files#diff-54056532b4b7aac8940fbec13725e98ceceb358bef02e1285edad2741dff83bdR159

mattcushman · 2023-06-14T22:25:46Z

src/libraries/external/KickerActions.sol

 );
+ // set LUP to current pool value
+ kickResult_.lup = vars.currentLup;


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.

makes sense, though should be done for regular kick too, will add this with a commit

done in _kick function for both types of kick https://github.com/ajna-finance/contracts/pull/894/files#diff-54056532b4b7aac8940fbec13725e98ceceb358bef02e1285edad2741dff83bdR368

…nalty Fix more tests

…h-deposit-re

prateek105

LGTM! One suggestion to rename kickWithDeposit to kickToRemoveDeposit.

ith-harvey

LGTM, have a comment regarding bankruptcy

ith-harvey · 2023-06-15T12:06:15Z

src/libraries/external/KickerActions.sol

- vars.bucketUnscaledDeposit = Deposits.unscaledValueAt(deposits_, index_);
- vars.bucketScale = Deposits.scale(deposits_, index_);
- vars.bucketDeposit = Maths.wmul(vars.bucketUnscaledDeposit, vars.bucketScale);
+ vars.lenderLP = bucket.bankruptcyTime < lender.depositTime ? lender.lps : 0;


kickWithDeposit could be called in the same block as bucket bankruptcy. It would use lender.lps even though the bucket is bankrupt. Recommend adding a revert like we have in moveQuoteToken ->

// cannot move in the same block when target bucket becomes insolvent if (vars.toBucketBankruptcyTime == block.timestamp) revert BucketBankruptcyBlock();`

if bucket bankruptcy happens in same block but after kick with deposit is called then vars.lenderLP will be 0 making entitled amount to be 0 and tx to revert with InsufficientLiquidity

I was thinking before, what if a bucket bankruptcy occurs in the same block but before kickWithDeposit is called.

I don't think block when kick happens is the constraint here but the block / time of last deposit?

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 : )

src/libraries/external/KickerActions.sol

ith-harvey

LGTM

mattcushman

lgtm

EdNoepel

I do not understand the gas numbers in the report. Why is there a kickWithDeposit post-change, and why did gas cost increase?

Otherwise LGTM.

EdNoepel · 2023-06-18T01:12:18Z

src/libraries/external/KickerActions.sol

- index_,
- vars.amountToDebitFromDeposit,
- vars.redeemedLP,
- kickResult_.lup


Quite happy to eliminate all this logic.

EdNoepel · 2023-06-18T01:46:57Z

tests/forge/unit/ERC20Pool/ERC20PoolLiquidationsMisc.t.sol


 _assertPool(
 PoolParams({
 htp: 0,
 lup: 0.000000099836282890 * 1e18,
- poolSize: 8_002.290256823112434920 * 1e18,
+ poolSize: 8_083.134379679494060567 * 1e18,


Makes sense because bond isn't coming out of deposit. Confirmed old pool size + bond escrowed (line 486) = new pool size.

MikeHathaway

LGTM

grandizzy · 2023-06-18T06:11:45Z

I do not understand the gas numbers in the report. Why is there a kickWithDeposit post-change, and why did gas cost increase?

Otherwise LGTM.

This PR also fix the issue where LUP wasn't recalculated after kick (that is with new debt including kick penalty) which is now done in _kick function for both kick and lenderKick functions. Hence gas increasing by additional Fenwick traversal
https://github.com/ajna-finance/contracts/pull/894/files#diff-54056532b4b7aac8940fbec13725e98ceceb358bef02e1285edad2741dff83bdR363

Post kickWithDeposit is an leftover as gas was taken before refactoring function to lenderKick, updated accordingly.

grandizzy and others added 2 commits June 3, 2023 19:39

Merge pull request #874 from ajna-finance/develop

b301603

Merge develop

Initial proposal: remove internal accounting

e76de2b

mattcushman reviewed Jun 14, 2023

View reviewed changes

Revert if bucket price is lower than current LUP, fix tests

e98167b

grandizzy force-pushed the kick-with-deposit-re branch from 3022ec8 to 586c305 Compare June 14, 2023 18:48

Reorg code and revert, fix tests

375f52d

grandizzy force-pushed the kick-with-deposit-re branch from 586c305 to 375f52d Compare June 14, 2023 18:59

grandizzy added Priority: Critical Status: Work In Progress Scope: Bug fix Scope: Edits /src labels Jun 14, 2023

Redo QT3 - bond escrowed are always guaranteed

7dd9939

mattcushman reviewed Jun 14, 2023

View reviewed changes

grandizzy added 2 commits June 15, 2023 02:45

Recalculate LUP after kick actions, with additional debt from kick pe…

a9a983c

…nalty Fix more tests

Fix commented test

670c21f

grandizzy mentioned this pull request Jun 15, 2023

Fixes for kickWithDeposit: check LUP not below HTP, and index not wit… #893

Closed

grandizzy changed the title ~~Bug fix: kickWithDeposit to not remove any deposit and to require bond to be posted~~ Bug fix: kickWithDeposit should not remove deposits and require bond to be posted Jun 15, 2023

grandizzy force-pushed the kick-with-deposit-re branch from 9e61b1b to 93d5f0f Compare June 15, 2023 00:41

Update kickWithDeposit Natspec, remove obsolete event

f38431e

grandizzy force-pushed the kick-with-deposit-re branch from 93d5f0f to f38431e Compare June 15, 2023 00:43

grandizzy added 2 commits June 15, 2023 08:01

Consistent way to return lup and pool debt

531d311

Merge branch 'develop', remote-tracking branch 'origin' into kick-wit…

cf9459e

…h-deposit-re

grandizzy changed the title ~~Bug fix: kickWithDeposit should not remove deposits and require bond to be posted~~ Bug fix: kickWithDeposit likely push LUP below HTP affecting collateralized loans Jun 15, 2023

grandizzy changed the title ~~Bug fix: kickWithDeposit likely push LUP below HTP affecting collateralized loans~~ Bug fix: kickWithDeposit likely push LUP below HTP affecting healthy loans Jun 15, 2023

grandizzy marked this pull request as ready for review June 15, 2023 06:47

grandizzy requested a review from mattcushman June 15, 2023 06:47

grandizzy added Status: Ready for Review and removed Status: Work In Progress labels Jun 15, 2023

grandizzy requested review from prateek105 and MikeHathaway June 15, 2023 06:48

grandizzy requested review from ith-harvey and EdNoepel June 15, 2023 06:48

prateek105 approved these changes Jun 15, 2023

View reviewed changes

ith-harvey reviewed Jun 15, 2023

View reviewed changes

Rename kickWithDeposit to lenderKickAuction

8843778

grandizzy force-pushed the kick-with-deposit-re branch from a733350 to 8843778 Compare June 16, 2023 05:22

Merge remote-tracking branch 'origin' into kick-with-deposit-re

cc83947

grandizzy requested a review from ith-harvey June 16, 2023 05:53

grandizzy added 3 commits June 16, 2023 08:56

Rename lenderKickAuction to lenderKick

f9ea0c6

Log lender kick in invariants

4a268bc

Merge remote-tracking branch 'origin' into kick-with-deposit-re

3ff9ca5

mattcushman reviewed Jun 16, 2023

View reviewed changes

src/libraries/external/KickerActions.sol Show resolved Hide resolved

ith-harvey approved these changes Jun 16, 2023

View reviewed changes

grandizzy added 2 commits June 17, 2023 10:37

Merge branch 'develop' into kick-with-deposit-re

ccfb7d7

Fix compile errors after merge

aebee03

mattcushman approved these changes Jun 17, 2023

View reviewed changes

EdNoepel approved these changes Jun 18, 2023

View reviewed changes

MikeHathaway approved these changes Jun 18, 2023

View reviewed changes

grandizzy merged commit 5693050 into develop Jun 18, 2023

grandizzy deleted the kick-with-deposit-re branch June 18, 2023 06:16

grandizzy mentioned this pull request Jul 11, 2023

Chinmay - KickerActions uses wrong check to prevent Kickers from using deposits below LUP for KIckWithDeposit sherlock-audit/2023-04-ajna-judging#113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fix: `kickWithDeposit` likely push `LUP` below `HTP` affecting healthy loans #894

Bug fix: `kickWithDeposit` likely push `LUP` below `HTP` affecting healthy loans #894

grandizzy commented Jun 14, 2023 •

edited

Loading

mattcushman Jun 14, 2023

grandizzy Jun 15, 2023

mattcushman Jun 14, 2023

grandizzy Jun 14, 2023

grandizzy Jun 15, 2023

prateek105 left a comment

ith-harvey left a comment •

edited

Loading

ith-harvey Jun 15, 2023

grandizzy Jun 15, 2023

ith-harvey Jun 15, 2023

grandizzy Jun 15, 2023

ith-harvey Jun 16, 2023

ith-harvey left a comment

mattcushman left a comment

EdNoepel left a comment

EdNoepel Jun 18, 2023

EdNoepel Jun 18, 2023

MikeHathaway left a comment

grandizzy commented Jun 18, 2023 •

edited

Loading

Bug fix: kickWithDeposit likely push LUP below HTP affecting healthy loans #894

Bug fix: kickWithDeposit likely push LUP below HTP affecting healthy loans #894

Conversation

grandizzy commented Jun 14, 2023 • edited Loading

Description of change

High level

Description of bug or vulnerability and solution

Contract size

Pre Change

Post Change

Gas usage

Pre Change

Post Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek105 left a comment

Choose a reason for hiding this comment

ith-harvey left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ith-harvey left a comment

Choose a reason for hiding this comment

mattcushman left a comment

Choose a reason for hiding this comment

EdNoepel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeHathaway left a comment

Choose a reason for hiding this comment

grandizzy commented Jun 18, 2023 • edited Loading

Bug fix: `kickWithDeposit` likely push `LUP` below `HTP` affecting healthy loans #894

Bug fix: `kickWithDeposit` likely push `LUP` below `HTP` affecting healthy loans #894

grandizzy commented Jun 14, 2023 •

edited

Loading

ith-harvey left a comment •

edited

Loading

grandizzy commented Jun 18, 2023 •

edited

Loading