0xDetermination - A borrower eligible for liquidation can pay an improperly large amount of fees, and may be unfairly liquidated

[sherlock-admin2]

0xDetermination

high

A borrower eligible for liquidation can pay an improperly large amount of fees, and may be unfairly liquidated

Summary

If a borrower is partially liquidated and then increases the collateral balance to avoid further liquidation, they will pay an improperly large amount of fees and can be unfairly liquidated.

Vulnerability Detail

The root cause is that partial emergency liquidation doesn't update accLoanRatePerSeconds (https://github.com/sherlock-audit/2024-02-leverage-contracts/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L660-L666).

If a borrower is partially liquidated, fees will be increased by the entire collateral amount (https://github.com/sherlock-audit/2024-02-leverage-contracts/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L604-L633):

            (collateralBalance, currentFees) = _calculateCollateralBalance(
                borrowing.borrowedAmount,
                borrowing.accLoanRatePerSeconds,
                borrowing.dailyRateCollateralBalance,
                accLoanRatePerSeconds
            );
            ...
            if (collateralBalance > 0) {
                ...
            } else {
                currentFees = borrowing.dailyRateCollateralBalance; //entire collateral amount
            }
            ...
            borrowing.feesOwed += _pickUpPlatformFees(borrowing.holdToken, currentFees);

When liquidation occurs right after becoming liquidatable, the collateralBalance calculation in repay() above will be a small value like -1; and essentially all the fees owed will be collected.

If the borrower notices the partial liquidation and wishes to avoid further liquidation, increaseCollateralBalance() can be called to become solvent again. But since the accLoanRatePerSeconds wasn't updated, the borrower will have to doubly pay all the fees that were just collected. This will happen if a lender calls harvest() or the loan is liquidated again. The loan can also be liquidated unfairly, because the collateralBalance calculated above will be much lower than it should be.

Impact

The borrower may pay too many fees, and it's also possible to unfairly liquidate the position.

Code Snippet

https://github.com/sherlock-audit/2024-02-leverage-contracts/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L660-L666
https://github.com/sherlock-audit/2024-02-leverage-contracts/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L604-L633

Tool used

Manual Review

Recommendation

Update accLoanRatePerSeconds for incomplete emergency liquidations.

[fann95]

accLoanRatePerSeconds should not be updated since borrowedAmount are reduced, accordingly, a position debt also redused
This issue was already discussed in the previous audit.
Harvest cannot be called on a position that is under liquidation

[nevillehuang]

Invalid, agree with sponsors comments. This was previously discussed here

[0xDetermination]

Hi @nevillehuang @fann95 , posting commented PoC with console.logs below (modified test).

The flow is like so:

1. Borrower's position is emergency liquidated by one lender
2. Borrower doesn't want to get liquidated more so increaseCollateralBalance() is called
3. Borrower will pay too many fees since dailyRateCollateralBalance was set to zero and accLoanRatePerSeconds wasn't updated

    it("emergency repay will be successful for PosManNFT owner if the collateral is depleted", async () => {
        let debt: ILiquidityBorrowingManager.BorrowingInfoExtStructOutput[] =
            await borrowingManager.getBorrowerDebtsInfo(bob.address);
        console.log('collateralBalance', debt[0].collateralBalance);
        console.log('current collateral amount', debt[0].info.dailyRateCollateralBalance);
        console.log('estimated life time', debt[0].estimatedLifeTime);
        await time.increase(debt[0].estimatedLifeTime.toNumber() + 1);

        debt = await borrowingManager.getBorrowerDebtsInfo(bob.address);
        console.log('collateralBalance after advancing time', debt[0].collateralBalance);

        let borrowingKey = (await borrowingManager.getBorrowingKeysForBorrower(bob.address))[0];
        let deadline = (await time.latest()) + 60;

        let params: ILiquidityBorrowingManager.RepayParamsStruct = {
            returnOnlyHoldToken: true,
            isEmergency: true, //emergency
            internalSwapPoolfee: 0,
            externalSwap: [],
            borrowingKey: borrowingKey,
            minHoldTokenOut: BigNumber.from(0),
            minSaleTokenOut: BigNumber.from(0)
        };

        await expect(borrowingManager.connect(alice).repay(params, deadline))
            .to.emit(borrowingManager, "EmergencyLoanClosure")
            .withArgs(bob.address, alice.address, borrowingKey);

        debt = await borrowingManager.getBorrowerDebtsInfo(bob.address);
        
        console.log('collateralBalance after first liquidation (this is wrong, should be close to zero)', debt[0].collateralBalance); //this amount is wrong, way too large due to the borrower's collateral set to zero and accLoanRatePerSeconds not updated. We can see that the amount actually increased instead of decreasing as it should
        
        //borrower increases collateral by a large amount such that liquidation shouldn't be possible anymore, 18000000000000000000000 (this is currently scaled by collateral precision, 1e18)
        //This amount is about 75% of the orignal collateral amount of 24948000000000000000000
        await borrowingManager.connect(bob).increaseCollateralBalance(borrowingKey, 18000n, deadline); //adjust amount for collateral balance precision
        //below should revert since collateral balance was increased by a large amount, but the borrower gets liquidated
        await expect(borrowingManager.connect(bob).repay(params, deadline))
            .to.emit(borrowingManager, "EmergencyLoanClosure")
            .withArgs(bob.address, bob.address, borrowingKey);
        
        await expect(borrowingManager.connect(owner).repay(params, deadline))
            .to.emit(borrowingManager, "EmergencyLoanClosure")
            .withArgs(bob.address, owner.address, borrowingKey);
    });

[fann95]

I see in the tests that the amount of debt has decreased by part of the closed Lp-share, but the rest of the debt has been maintained, as expected.
collateralBalance after advancing time BigNumber { value: "-363497523148146947" }
collateralBalance after first liquidation (this is wrong, should be close to zero) BigNumber { value: "-263884816316264683" }
debt(263884816316264683) < debt(363497523148146947)

[0xDetermination]

@fann95 That's weird, when I run in the contest repo the output is:
collateralBalance BigNumber { value: "6236502752476851853053" }
current collateral amount BigNumber { value: "24948000000000000000000" }
estimated life time BigNumber { value: "21598" }
collateralBalance after advancing time BigNumber { value: "-181748761574072227" }
collateralBalance after first liquidation (this is wrong, should be close to zero) BigNumber { value: "-18111392620173611109770" }

Also, not sure if the test is passing on your end, but if it's passing I think the bug is there since the collateral is increased and emergency liq shouldn't happen

[fann95]

It looks like this is a valid issue..I got different results since I ran your test in the updated version. The PR which corrected the problems with the distribution of commissions also corrected this problem.

[sherlock-admin]

The protocol team fixed this issue in PR/commit RealWagmi/wagmi-leverage@84416fc.

[nevillehuang]

@fann95 Is the root cause stemming from similar issues in #41 or only possible because of another issue, given fix PR is the same?

[0xDetermination]

@fann95 @nevillehuang Fix code looks good to me, PoC doesn't pass in the main repo.

In case my input is helpful- I think this issue is different than #41 since that issue describes a root cause/fix in borrow(), whereas the cause/fix for this issue is around not setting dailyRateCollateralBalance to zero in repay(). Additionally the PR is quite large and changes a lot of things.

[nevillehuang]

@fann95 @0xDetermination I am trying to figure out how the original fix could have fixed this issue without first considering it, which leads me to believe they share the same root causes revolving around distribution of fees. I would have to take a closer look at the fix PR.

[fann95]

@fann95 @0xDetermination I am trying to figure out how the original fix could have fixed this issue without first considering it, which leads me to believe they share the same root causes revolving around distribution of fees. I would have to take a closer look at the fix PR.

This problem is indirectly related to the distribution of commissions. I got rid of the mechanism for accumulating a fee, therefore the current error was fixed.

[nevillehuang]

@Czar102 @0xDetermination What are your thoughts here based on duplication rules here? The core vulnerability seem to stem from erroneous distribution of fees which allowed for this issue to be possible in the first place. I am inclined to think this should be duplicated with #41

There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths:

• B -> high severity path
• C -> medium severity attack path/just identifying the vulnerability.
  Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates.

[0xDetermination]

@nevillehuang @Czar102 The root cause and fix for this issue are both distinct from #41- the erroneous distribution of fees in #41 is caused by not harvesting fees when a new loan is taken with the borrow() function, and the fix is the new internal _harvest() function that runs in borrow(). (link)

This issue #40 is caused by the fee distribution mechanism in emergency liquidation mode in repay(), which is separate from borrow() and harvest(). The root cause and fix can both be seen here in repay(). The fix for #41 won't fix this issue.

[nevillehuang]

Since #39 and #41 talks about different types of fees, I agree they are not duplicates and they are both high severity findings. Given this erroneous fee calculation affects a large portion of the protocol, I agree with sponsor comments here and believe high severity is appropriate.

However, I believe that #40 is a medium severity issue only possible because of the root cause of wrong computation of fees for borrowed positions within #41. This is evident from the fix employed without considering this issue in the first place. Hence, I am going to duplicate it with #41 and assign it as high severity based on sherlock rules despite it having only a medium severity impact.

[0xDetermination]

Escalate

I understand @nevillehuang's point here, but I still think this shouldn't be a dup for the reasons I gave in my above comment. The fix PR changed a lot of things unrelated to validated issues, such as removal of the min fee mechanism.

Will appreciate @Czar102's decision.

Additionally, if validated as not a dup, not sure if this should be H or M based on the 'external conditions' criteria.

[sherlock-admin2]

Escalate

I understand @nevillehuang's point here, but I still think this shouldn't be a dup for the reasons I gave in my above comment. The fix PR changed a lot of things unrelated to validated issues, such as removal of the min fee mechanism.

Will appreciate @Czar102's decision.

Additionally, if validated as not a dup, not sure if this should be H or M based on the 'external conditions' criteria.

You've created a valid escalation!

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.

[Czar102]

@nevillehuang can you elaborate on how does a fix to #40 follows from #41?

[nevillehuang]

@Czar102 I believe @fann95 answered it here and here

Based on sponsor description and comments above, The PR fix was performed without consideration of this issue, which led me to believe this issue only stems from the root cause of incorrect distribution.

[Czar102]

@nevillehuang what is the single logical error (maybe an assumption, maybe approach) that led to both of these issues?

Having issue A which makes the sponsor want to restructure code and it accidentally removing issue B doesn't make them duplicates.

[fann95]

I propose to confirm this issue and not consider it a duplicate.

[nevillehuang]

@Czar102 It stems from the logic for fee distribution. Although I disagree, seems like sponsor agrees to deduplicate so we can proceed with deduplication.

[Czar102]

@nevillehuang regarding this:

It stems from the logic for fee distribution.

I believe it doesn't answer my question:

what is the single logical error (maybe an assumption, maybe approach) that led to both of these issues?

I wanted to have a one-sentence description of the common ground of these issues, and the fact that the issues "stem from the logic for fee distribution" (are in the same part of the code logic) doesn't make them duplicates.

I'm planning to make this issue a unique issue, unless a justification (as mentioned above) is provided.

What are the considerations regarding the severity? @nevillehuang
@0xDetermination what did you mean by the following fragment of your escalation?

not sure if this should be H or M based on the 'external conditions' criteria

[0xDetermination]

@Czar102 I'm not 100% sure whether this is better suited for H or M, as I don't have a ton of experience with Sherlock judging rules. Basically, the issue can cause serious loss of funds (borrower's entire collateral), but it is conditional on a partial emergency liquidation followed by the borrower increasing collateral. It looks more like M to me but I don't want to speak too soon, will leave it up to you and @nevillehuang. Happy to provide more info if needed.

[nevillehuang]

Yup @Czar102 I agree with your decision, I cannot pinpoint an exact singular approach/code logic given this is an update contest and would take up too much time. The reason I duplicated them was on the side of caution, given the sponsor quite literally fix this issue without even considering it. But since sponsor also agree with deduplication, lets move ahead

[Czar102]

In that case, planning to make this issue a unique Medium severity one.

[Czar102]

Result:
Medium
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xDetermination: accepted

[0xDetermination]

Fix looks good, collection of fees in emergency mode repay() has been reworked. Notably, dailyRateCollateralBalance is no longer set to zero.

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.