-
Notifications
You must be signed in to change notification settings - Fork 3
Chinmay - KickerActions uses wrong check to prevent Kickers from using deposits below LUP for KIckWithDeposit #113
Comments
Lead Watson comment:
|
Escalate for 10 USDC 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 : The LUP is being calculated as if the debt has increased by an amount, for example :
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. |
Escalate for 10 USDC To clarify:
|
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. |
I'd like to elaborate the impact to justify High severity :
|
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 |
Agree with @dmitriia explanation and medium severity. |
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. |
fixed with ajna-finance/ajna-core#894 |
Looks ok, as a result of refactoring (for this and other issues of this contest combined) |
Result:
|
Escalations have been resolved successfully! Escalation status:
|
Chinmay
high
KickerActions uses wrong check to prevent Kickers from using deposits below LUP for KIckWithDeposit
Summary
The
kickWithDeposit
function inKickerActions
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 thebucketPrice
used.Consider a situation :
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.
The text was updated successfully, but these errors were encountered: