Chinmay - KickerActions uses wrong check to prevent Kickers from using deposits below LUP for KIckWithDeposit

[sherlock-admin]

Chinmay

high

KickerActions uses wrong check to prevent Kickers from using deposits below LUP for KIckWithDeposit

Summary

The kickWithDeposit function in KickerActions has a check to prevent users having deposits below the LUP to use those deposits for kicking loans, but this check is implemented incorrectly.

Vulnerability Detail

The mentioned check evaluates if the bucketPrice used by the kicker is below the LUP. But the problem here is that the LUP used in this check is the new LUP that is calculated after incorporating the removal of the deposit itself and the debt changes. Thus, this check can be easily bypassed because the new LUP is bound to move lower and thus may cross past the bucketPrice used.

Consider a situation :

1. The kicker has deposits in a bucketPrice below LUP
2. Now he calls kickWithDeposit using this deposit
3. The new LUP is calculated after removing this deposit and adding the debt changes for kickPenalty etc.
4. This will make the LUP decrease and thus now LUP < bucketPrice that the user input

This way this check can be bypassed. According to the developers, the check was in place to prevent kickers from using deposits below the LUP to kick loans but this is not fulfilled.

Impact

This breaks protocol functionality because now anyone can deposit below LUP and use that to kick valid user loans. The kickWithDeposit function was only made to help depositors get their deposits back if some loans are blocking the withdrawl due to the LUP movement. But the current implementation allows anyone to kick those loans that were originally not eligible for liquidation.

This is a high severity issue because it griefs users off their funds by liquidating them, even when they were not eligible for liquidation.

Code Snippet

https://github.com/sherlock-audit/2023-04-ajna/blob/e2439305cc093204a0d927aac19d898f4a0edb3d/ajna-core/src/libraries/external/KickerActions.sol#L216

Tool used

Manual Review

Recommendation

Refactor the code and move this check to the top of the kickWithDeposit function logic.

[hrishibhat]

Lead Watson comment:

Invalid as removal deposit from the bucket below the LUP can’t make LUP move, i.e. it can’t be vars.bucketPrice < kickResult_.lup before removal from bucket, but vars.bucketPrice >= kickResult_.lup after that.

[chinmay-farkya]

Escalate for 10 USDC
The lead watson's comment "removal deposit from the bucket below the LUP can’t make LUP move" is incorrect.

Following up with the explanation above, see that at KickerActions#216 the LUP that is used for the check has been calculated at KickerActions#207. Here, the DepositState "deposits_" is the actual old state of deposits which means that kicker's deposit is still stored at whatever index it was. This deposit is actually only removed from "deposits_" at KickerActions#223 and KickerActions#241 which means the deposits used for kick is only removed from the "deposits_" after the check at Line 216.

Now look at the values in Line 207 : the LUP calculation using "deposits_" state where the user's deposit he is going to use to kick exists, but see the second function argument : kickResult_.lup = Deposits.getLup(deposits_, poolState_.debt + vars.amountToDebitFromDeposit)

The LUP is being calculated as if the debt has increased by an amount, amountToDebitFromDeposit which is mirroring the removal of the deposit. But think about when the deposit index used (vars.bucketPrice) was originally below the LUP. In this case, the use of increased debt in the calculation doesn't mirror the removal of deposits.

for example :

1. LUP = 100 and kicker uses bucket price 90 where he has 100K deposits.
2. Now this shouldn't move the LUP but it does move the LUP in calculation at Line 207 because it uses the same old Deposit but increased Debt(by whole of the deposits used for kick) in the calculation. Since LUP is calculated by summing up the deposits from top and matching it against the debt value passed to the getLup function, this will lower the LUP. This incorrect LUP will bypass the check and allow anyone to kick loans and grief users as explained in the original submission. This can be seen in Deposits.getLup function.

Because the logic can move the LUP below despite of a deposit from below the LUP being removed, this is a valid High severity finding as explained in impact above.

[sherlock-admin]

Escalate for 10 USDC
The lead watson's comment "removal deposit from the bucket below the LUP can’t make LUP move" is incorrect.

Following up with the explanation above, see that at KickerActions#216 the LUP that is used for the check has been calculated at KickerActions#207. Here, the DepositState "deposits_" is the actual old state of deposits which means that kicker's deposit is still stored at whatever index it was. This deposit is actually only removed from "deposits_" at KickerActions#223 and KickerActions#241 which means the deposits used for kick is only removed from the "deposits_" after the check at Line 216.

Now look at the values in Line 207 : the LUP calculation using "deposits_" state where the user's deposit he is going to use to kick exists, but see the second function argument : kickResult_.lup = Deposits.getLup(deposits_, poolState_.debt + vars.amountToDebitFromDeposit)

The LUP is being calculated as if the debt has increased by an amount, amountToDebitFromDeposit which is mirroring the removal of the deposit. But think about when the deposit index used (vars.bucketPrice) was originally below the LUP. In this case, the use of increased debt in the calculation doesn't mirror the removal of deposits.

for example :

1. LUP = 100 and kicker uses bucket price 90 where he has 100K deposits.
2. Now this shouldn't move the LUP but it does move the LUP in calculation at Line 207 because it uses the same old Deposit but increased Debt(by whole of the deposits used for kick) in the calculation. Since LUP is calculated by summing up the deposits from top and matching it against the debt value passed to the getLup function, this will lower the LUP. This incorrect LUP will bypass the check and allow anyone to kick loans and grief users as explained in the original submission. This can be seen in Deposits.getLup function.

Because the logic can move the LUP below despite of a deposit from below the LUP being removed, this is a valid High severity finding as explained in impact above.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[dmitriia]

Escalate for 10 USDC
Despite some misunderstandings, the take that kickResult_.lup = Deposits.getLup(deposits_, poolState_.debt + vars.amountToDebitFromDeposit) is incorrect when bucketPrice is initially below LUP is a right one, i.e. the control do not filter out some bucketPrice < initial_LUP situations as LUP for the check isn't calculated correctly in this case.
However, the filtering of bucketPrice < LUP is a more a convenience approach. There is no impact here that justify high severity, so valid medium looks the most suitable in this case.

To clarify:

1. "removal deposit from the bucket below the LUP can’t make LUP move" is correct all the time, this follows from the definition of LUP, which is the last utilized price, while bucketPrice < LUP deposits aren't utilized.
2. The kickResult_.lup = Deposits.getLup(deposits_, poolState_.debt + vars.amountToDebitFromDeposit) check isn't correct exactly because of (1)

[sherlock-admin]

Escalate for 10 USDC
Despite some misunderstandings, the take that kickResult_.lup = Deposits.getLup(deposits_, poolState_.debt + vars.amountToDebitFromDeposit) is incorrect when bucketPrice is initially below LUP is a right one, i.e. the control do not filter out some bucketPrice < initial_LUP situations as LUP for the check isn't calculated correctly in this case.
However, the filtering of bucketPrice < LUP is a more a convenience approach. There is no impact here that justify high severity, so valid medium looks the most suitable in this case.

To clarify:

1. "removal deposit from the bucket below the LUP can’t make LUP move" is correct all the time, this follows from the definition of LUP, which is the last utilized price, while bucketPrice < LUP deposits aren't utilized.
2. The kickResult_.lup = Deposits.getLup(deposits_, poolState_.debt + vars.amountToDebitFromDeposit) check isn't correct exactly because of (1)

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[chinmay-farkya]

I'd like to elaborate the impact to justify High severity :

1. Firstly, this allows an attacker to intentionally deposit below the LUP and force illegal liquidations on users. Since the loan with max TP is kicked first in order, this may cause a material loss of funds for that borrower depending upon his borrowed amounts.
2. The attacker can do this repeatedly to grief multiple users one by one. This may cause a material loss of funds for multiple users.
3. The attacker controls the deposited amount ie. amountToDebitFromDeposit above based on how much LP he holds in that bucket. This way he can move the LUP (for the intermediate calculation) lower to any price he desires. This is because larger the amountToDebitFromDeposit more the LUP will move. This means he can forcefully liquidate users even if they were far from underwater.

[0xffff11]

There are several points were the watson was mistaken. I really appreciate the comment from the senior exposing them. I do agree with a medium in the current landscape, but I would like a third opinion from @grandizzy

[grandizzy]

Agree with @dmitriia explanation and medium severity.

[chinmay-farkya]

Hey @grandizzy with regards to the Senior Watson's comments, I think I have justified the severity enough in my comment here

Do have a look. Won't take this further.

Also, Hey @0xffff11 would like to ask for more clarification on why the points I made in the last comment above aren't fulfilling to high severity.

[grandizzy]

fixed with ajna-finance/ajna-core#894

[dmitriia]

https://github.com/sherlock-audit/2023-04-ajna/blob/e2439305cc093204a0d927aac19d898f4a0edb3d/ajna-core/src/libraries/external/KickerActions.sol#L216

Looks ok, as a result of refactoring (for this and other issues of this contest combined) kickWithDeposit() is now lenderKick() that performs if (vars.bucketPrice < Deposits.getLup(deposits_, poolState_.debt)) revert PriceBelowLUP() check on the initial pool state.

[hrishibhat]

Result:
Medium
Unique
In addition to the comments above after further review and discussion of this issue, Considering this issue a valid medium based on to the following comments from the Lead Watson:

Depositing below LUP is definitely possible in general and is not enabled by this bug, what happens here is that LUP for kicking determined as Deposits.getLup(deposits_, poolState_.debt + vars.amountToDebitFromDeposit) is incorrect for deposits below LUP (and for them only, it is correct whenever bucket price >= LUP). This is actually an intermediary bug, I discussed it with the team early in the contest, but didn’t reported as it led to several others so the team decided to rewrite the function altogether (it’s now much lighter lenderKick()), and this requirement was initially introduced as a function separation matter, i.e. there is nothing wrong in using kicking with a bucket deposit when vars.bucketPrice < kickResult_.lup, it is just designed to handle an another case of a depositor pretending to remove the deposit and kicking as if they done that. I.e. that’s basically a helper function that can be replicated via others (withdraw, then kick).

Why it is medium: first of all kicking cannot be sniped and is always done in queue, one can kick only Loans.getMax(loans_).borrower, there is no optionality here, so if there are lots of bad borrowers the attacker will have to kick them first anyway, they can’t just reach a good one, that’s not possible, and, most importantly, kicking borrowers that aren’t supposed to be kicked is substantially unprofitable, as kicker have to post a bond, which is partially forfeited if resulting auction price (i.e. market price as market actors will participate there as long as it’s profitable) is higher than this borrower’s price. The loss of the kicker is proportional to this gap, i.e. there is a guaranteed (as long as there are other market participants) loss for the kicker is they kick healthy borrowers, and the healthier is the borrower, the bigger the loss.

So yes, an attacker can kick with this function, but it will be really costly griefing and nothing else. I.e. they will be required to post substantial bond and will lose a part of it, while borrower will not lose as they will be liquidated at market price, i.e. their position will be just closed at the current levels. I.e. nobody besides attacker will have any substantial losses, attacker will be penalized for kicking at out of the market levels, their penalty will be redistributed.

This way this can be categozed as costly griefing, so the probability of such attack is low, while damage cannot be made significant (i.e. there can be some medium damage for borrower if market is specifically illiquid, but active market is basically a precondition for oracle-less protocols, and this situation cannot be controlled by the attacker).

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• chinmay-farkya: accepted
• dmitriia: accepted