0xDetermination - Entrance fees are distributed wrongly in loans with multiple lenders

[sherlock-admin]

0xDetermination

high

Entrance fees are distributed wrongly in loans with multiple lenders

Summary

Entrance fees are distributed improperly, some lenders are likely to lose some portion of their entrance fees. Also, calling updateHoldTokenEntranceFee() can cause improper entrance fee distribution in loans with multiple lenders.

Vulnerability Detail

Note that entrance fees are added to the borrower's feesOwed when borrowing:

        borrowing.feesOwed += entranceFee;

Also note that the fees distributed to each lender are determined by the following calculation (https://github.com/sherlock-audit/2024-02-leverage-contracts/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L546-L549):

                uint256 feesAmt = FullMath.mulDiv(feesOwed, cache.holdTokenDebt, borrowedAmount); //fees owed multiplied by the individual amount lent, divided by the total amount lent
                ...
                loansFeesInfo[creditor][cache.holdToken] += feesAmt;
                harvestedAmt += feesAmt;

This is a problem because the entrance fees will be distributed among all lenders instead of credited to each lender. Example:

1. A borrower takes a loan of 100 tokens from a lender and pays an entrance fee of 10 tokens.
2. After some time, the lender harvests fees and fees are set to zero. (This step could be frontrunning the below step.)
3. The borrower immediately takes out another loan of 100 tokens and pays and entrance fee of 10 tokens.
4. When fees are harvested again, due to the calculation in the code block above, 5 tokens of the entrance fee go to the first lender and 5 tokens go to the second lender. The first lender has collected 15 tokens of entrance fees, while the second lender has collected only 5- despite both loans having the same borrowed amount.

Furthermore, if the entrance fee is increased then new lenders will lose part of their entrance fee. Example:

1. A borrower takes a loan of 100 tokens from a lender and pays an entrance fee of 10 tokens.
2. The entrance fee is increased.
3. The borrower increases the position by taking a loan of 100 tokens from a new lender, and pays an entrance fee of 20 tokens.
4. harvest() is called, and both lenders receive 15 tokens out of the total 30 tokens paid as entrance fees. This is wrong since the first lender should receive 10 and the second lender should receive 20.

   Impact

   Lenders are likely to lose entrance fees.

   Code Snippet

   https://github.com/sherlock-audit/2024-02-leverage-contracts/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L1036 https://github.com/sherlock-audit/2024-02-leverage-contracts/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L546-L549

   Tool used

Manual Review

Recommendation

Could add the entrance fee directly to the lender's fees balance instead of adding it to feesOwed, and then track the entrance fee in the loan data to be used in min fee enforcement calculations.

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/RealWagmi/wagmi-leverage/commit/84416fcedfcc7eb062917bdc69f919bba9d3c0b7.

[fann95]

Yes, the problem existed and is associated with the same error as #41. This issue is related to an erroneous scheme for accumulating fees and affected almost all functions in the contract, so the PR turned out to be quite large.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid; high(1)

[nevillehuang]

@fann95 Is the root cause the same as #41?

[fann95]

I think so since it was assumed that the entrance fee would be distributed the same way as the fees for borrowing.

[nevillehuang]

@fann95 Can you take a look at this comment and let me know your thoughts

[fann95]

Can you take a look at this comment and let me know your thoughts

done

[nevillehuang]

See comments here

[zrax-x]

Escalate

This should be a duplicate of issue 41, or be of Medium severity.

Here are my two reasons.

Firstly, all of these issues (issue 41, issue 16 and this one) discuss the distribution of fees. In both issue 41 and issue 16, it was mentioned that the fees are not distributed in the function borrow, causing some lenders to lose fees. This issue talks about the same fee distribution problem, except it focuses on the entrance fees. In that respect, it's a Duplicate.

Secondly, entrance fees are a fraction of all fees, only 0.1% is charged by default and can be set to 0. Therefore, its impact is obviously not as serious as that mentioned in issue 41. I consider this to be a Medium severity issue.

[sherlock-admin2]

Escalate

This should be a duplicate of issue 41, or be of Medium severity.

Here are my two reasons.

Firstly, all of these issues (issue 41, issue 16 and this one) discuss the distribution of fees. In both issue 41 and issue 16, it was mentioned that the fees are not distributed in the function borrow, causing some lenders to lose fees. This issue talks about the same fee distribution problem, except it focuses on the entrance fees. In that respect, it's a Duplicate.

Secondly, entrance fees are a fraction of all fees, only 0.1% is charged by default and can be set to 0. Therefore, its impact is obviously not as serious as that mentioned in issue 41. I consider this to be a Medium severity issue.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[qmdddd]

Escalate

See above.

[sherlock-admin2]

Escalate

See above.

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.

[0xDetermination]

I disagree with the escalation's first point for the following reasons:

1. The root cause/fix for this issue is distinct from the root cause/fix for 41 (these fixes are implementing the recommendations). Fixing one issue won't fix the other.
2. Entrance fees are a different type of fees than fees accumulated over time.

As for the second point, I agree that this may be a borderline M, but I lean more towards H for the following reasons:

1. Entrance fees can be set up to 10% of the borrowed amount, which can be quite a lot considering that this is a high leverage protocol.
2. I'm not sure if the loss here can be called 'highly constrained' considering that a lot of net loss can build up over time as more positions are taken in the protocol. (This issue will occur in every loan with multiple lenders.) For example, if over a period of 1 year there are 1000 multi loan positions taken by users in the protocol with $100 worth of entrance fees lost in each position, then the net loss/misallocation from this bug could be $100,000.
3. A malicious borrower can borrow from himself or a colluding lender and then borrow from another lender to steal entrance fees (making the second position cheaper)

Would like to see how @Czar102 judges this issue, since I do think the severity may be borderline.

[nevillehuang]

@zrax-x Can you provide code logic to prove the root cause is similar? If not I think it should remain not as duplicates given it involves different fee types.

Additionally, I believe medium severity could be more appropriate, but based on what @fann95 has highlighted, the impact is widespread throughout the whole system. @fann95 Could you shed some light on the potential impact it could have and does it justify high severity?

[zrax-x]

I believe High severity is not appropriate.

The entry fee rate defaults to 0.1% and entry fee is determined when borrowing (i.e. it does not increase over time), so its amount is limited.

At the same time, in order to steal the entry fees, the attacker will have to pay fees to the platform (as implemented in the function _pickUpPlatformFees), which accounts for 20% of the interest fees. So the attack cost is very high.

    /**
     * @dev Platform fees in basis points.
     * 2000 BP represents a 20% fee on the daily rate.
     */
    uint256 public platformFeesBP = 2000;

In summary, this issue will indeed cause some lenders to lose part of entry fees, but its impact is limited. I maintain it is a Medium severity issue.

[0xDetermination]

I think an impact that might make this issue H without needing to consider conditions like entrance fee settings is the 'net loss built up over time' example in my previous comment (edited to add the example).

Addressing @zrax-x's point about platform fees- true, there is a maximum/default 20% platform fee although it can also be set lower. If we assume slippage is negligible the attack would still be profitable, but I agree that this would reduce the profitability.

[Czar102]

@zrax-x @qmdddd can you follow up on @nevillehuang's question?

Can you provide code logic to prove the root cause is similar?

Just want to have clarity on the duplication status before considering the severity. Do you agree that this issue shouldn't be a duplicate of #41?

[zrax-x]

@Czar102 I now believe this can be a distinct issue, although it also pertains to fee distribution, the distinction lies in the calculation methods for the fees.

[Czar102]

@zrax-x @qmdddd @0xDetermination what is the optimal attacker's strategy to minimize their fees? How much does the lender lose in that scenario?

[0xDetermination]

@Czar102 @zrax-x I looked into it more and I think the minimizing fee attack is actually not profitable unless the attacker is frontrunning the admin increasing entrance fees or is colluding with another lender that the attacker legitimately wants to borrow from. So I think the highest impact for this issue may be the 'net loss' scenario as described in my earlier comment, considering that this issue will occur for every loan with multiple lenders.

[zrax-x]

@Czar102 Yes, I agree with @0xDetermination. First of all, it is difficult for attackers to profit from it because attackers need to pay relatively expensive platform fees. Secondly, some lenders will indeed lose a certain amount of entry fee, but I think this loss is small (considering that the entry fee rate is 0.1%, and it is not a complete loss).

[0xDetermination]

@zrax-x Yes, but the entrance fee can be up to 10%, and even with a 0.1% fee rate a large amount of net loss can accumulate over time- which is why I think this issue could be borderline.

[zrax-x]

My opinion is that considering that the root cause of this issue and issue#41 are the same (both use the same distribution method, as @fann95 commented before), the difference is only in the fee calculation. At the same time, the impact of issue#41 is more serious (the attacker is profitable and the loss is greater). So, on both counts, I believe that its severity should be M.

[Czar102]

Because of the heavy constraints on the exploitability (is never profitable based on what was said), and the fact that (from my understanding) the goal of this fee is mainly to prevent extremely short-term borrows and not to increase lenders' earnings, I believe this is a Medium severity issue.

Planning to accept the escalation and downgrade the issue to Medium.

[Czar102]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• qmdddd: accepted

[0xDetermination]

Fix looks good, fee collection has been reworked and entrance fees are added directly to the lender's reward balance.

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.