Incorrect assumption of loss in liquidatePositionBadDebt(), leads to undue burning of Treasury shares

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

Undue burning of Treasury shares in PoolV3 leading to loss of profit to stakers, protocol and dLp lockers.

Proof of Concept

1. First thing to note is that, CDPVault records profit for every interest paid. see proofs;
   In liquidatePosition()

if (deltaDebt == maxRepayment) { 
            newDebt = 0;
            newCumulativeIndex = debtData.cumulativeIndexNow;
            profit = debtData.accruedInterest; // during full liquidation
            position.cumulativeQuotaInterest = 0; 

during full repayment

 } else if (deltaDebt < 0) {
            uint256 maxRepayment = calcTotalDebt(debtData);
            uint256 amount = abs(deltaDebt);
            if (amount >= maxRepayment) {
                amount = maxRepayment; // U:[CM-11]
                deltaDebt = -toInt256(maxRepayment);
            }

            poolUnderlying.safeTransferFrom(creditor, address(pool), amount);

            uint128 newCumulativeQuotaInterest;
            if (amount == maxRepayment) {
                newDebt = 0;
                newCumulativeIndex = debtData.cumulativeIndexNow;
                profit = debtData.accruedInterest;
                newCumulativeQuotaInterest = 0;

It can also be observed in calDecrease() that when cumulativeQuotaInterest or accumulatedInterest is repaid, profit is recorded. here

2. Now that it is proven that, interest is being recorded as profit and shares are minted in repayCreditAccount() to treasury, let us examine what was done wrong in liquidatePositionBadDebt()
3. The liquidatePositionBadDebt() incorrectly assumes that, once the repayAmount is less than totalDebt, a loss occurs, this is not correct in all cases.

Instance

accrued Interest    =========== cumulativeQuotaInterest + accruedInterest = 200
total debt value    =========== 1,200(debt + accruedInterest)
collateral value    =========== 1,166 (debt > collater)
repayAmount         =========== 1,050
discountPrice       =========== 0.9
takeCollateral.     =========== 1,166

From the snippet above, it shows that, debt is 1,000 and liquidator repaid 1,050, which leaves 50 as part of the interest payment and shares should be minted to the treasury for that. But the current implementation of liquidatePositionBadDebt() does other wise, by burning treasury shares worth 1,200 - 1,050 = 150. In the real sense, 150 was not lost here, 50 was gained as part interest payment!

@>>     uint256 loss = calcTotalDebt(debtData) - repayAmount;

        // transfer the repay amount from the liquidator to the vault
        poolUnderlying.safeTransferFrom(msg.sender, address(pool), repayAmount);

        position.cumulativeQuotaInterest = 0;
        position.cumulativeQuotaIndexLU = debtData.cumulativeQuotaIndexNow;
        // update liquidated position
        position = _modifyPosition(
            owner,
            position,
            0,
            debtData.cumulativeIndexNow,
            -toInt256(takeCollateral),
            totalDebt
        );

 @>>       pool.repayCreditAccount(debtData.debt, 0, loss); // U:[CM-11]

Tools Used

Manual review

Recommended Mitigation Steps

check if the repaid amount is greater or less than the actual debt, if it is greater, the difference should be minted as profit to the treasury, if lower, difference should be burnt from treasury.

Assessed type

Math

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto marked the issue as duplicate of #190

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)