Wrong calculation of newCumulativeIndex in case of partial borrowing interest repayment

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/CDPVault.sol#L707
https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/CDPVault.sol#L670

Vulnerability details

When debt on a position is repaid (or via liquidation), first the quotaInterest is reduced and then the base interest is reduced. In the case that quotaInterest has been fully paid with the provided amount and the remaining fund in this call can only cover the base borrowing interest partially, a newCumulativeIndex is calculated using a formula such that the accrued interest can be counted correctly later too. See here

This logic can be seen in CDPVault : calcDecrease() function.

But the calculation of the new index for the position is incorrect.

It uses "profit" variable as the "delta" debt that is paid out of the borrowing interest. But in current logic, "profit" may also include the quotaInterest that was paid for in the same repay call. Inclusion of quotaInterest in the index calculation of borrowing interest is completely wrong due to the following reasons :

1. The code is adapted from gearbox creditManager mechanism. In gearbox's library creditlogic.sol (ie. used by creditManager of gearbox to calculate the new Index, the same thing CDPVault is trying to do in Loopfi) : calcDecrease() function], we can see that the "delta" used in newCumulativeIndex calculation is only the amount that is used for paying borrowing interest partially, and "does not include the quota Interest in any way".

In creditlogic's context amountToPool is deduced from amountToRepay which is basically borrowing interest repaid here and then amountToPool is then used in calculating newcumulativeIndex.

2. quota interest and borrowing interest are two separate components of debt. Their repayment and increase/decrease happens via two separate mechanisms. quotaInterest value has no business in being present in newCumulativeIndex calculation, where newCumulativeIndex is going to be used for tracking the base borrowing interest down the line.

Impact

From the equation it can be deduced that newCumulativeIndex is directly prop. to profit repaid. So everytime quotaInterest and borrowing interest are paid in the same call, this profit variable will have larger that desired value, which will lead to a higher newCumulativeIndex value.

Then anytime later when the user repays debt again, he will have to pay less debt than the real debt (because accruedInterest depends on difference in current index and last updated index). A portion of his debt gets unintendedly written off due to the wrong calculation.

Attackers can use this to intentionally write off their debt. All this stems from the wrong calculation of the interest delta.

Proof of Concept

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/CDPVault.sol#L707

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/CDPVault.sol#L670

Tools Used

Manual review

Recommended Mitigation Steps

Add a new variable that tracks only the "accrued borrowing interest paid" in the current call (similar to how its done for quotaInterest here), and use that as "delta" in the newCumulativeIndex formula. This will correctly exclude any quotaInterest payments from the newCumulativeIndex calculation.

Assessed type

Context