In CDPVault::liquidatePositionBadDebt(), the calculation of loss is incorrect.

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

The incorrect calculation affects the protocol’s profit assessment, resulting in potential losses for users, particularly in the interest portion.

Proof of Concept

    function liquidatePositionBadDebt(address owner, uint256 repayAmount) external whenNotPaused {
        // validate params
        if (owner == address(0) || repayAmount == 0) revert CDPVault__liquidatePosition_invalidParameters();

        // load configs
        VaultConfig memory config = vaultConfig;
        LiquidationConfig memory liqConfig_ = liquidationConfig;

        // load liquidated position
        Position memory position = positions[owner];
        DebtData memory debtData = _calcDebt(position);
        uint256 spotPrice_ = spotPrice();
        if (spotPrice_ == 0) revert CDPVault__liquidatePosition_invalidSpotPrice();
        // verify that the position is indeed unsafe
        if (_isCollateralized(calcTotalDebt(debtData), wmul(position.collateral, spotPrice_), config.liquidationRatio))
            revert CDPVault__liquidatePosition_notUnsafe();

        // load price and calculate discounted price
        uint256 discountedPrice = wmul(spotPrice_, liqConfig_.liquidationDiscount);
        // Enusure that the debt is greater than the collateral at discounted price
        if (calcTotalDebt(debtData) <= wmul(position.collateral, discountedPrice)) revert CDPVault__noBadDebt();
        // compute collateral to take, debt to repay
        uint256 takeCollateral = wdiv(repayAmount, discountedPrice);
        if (takeCollateral < position.collateral) revert CDPVault__repayAmountNotEnough();

        // account for bad debt
        takeCollateral = position.collateral;
        repayAmount = wmul(takeCollateral, discountedPrice);
    @>>    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]
        // transfer the collateral amount from the vault to the liquidator
        token.safeTransfer(msg.sender, takeCollateral);

        int256 quotaRevenueChange = _calcQuotaRevenueChange(-int(debtData.debt));
        if (quotaRevenueChange != 0) {
            IPoolV3(pool).updateQuotaRevenue(quotaRevenueChange); // U:[PQK-15]
        }
    }

In the liquidatePositionBadDebt function, the calculation of the loss is done by subtracting the repaid portion of the debt from the total debt.

    function calcTotalDebt(DebtData memory debtData) internal pure returns (uint256) {
@>>        return debtData.debt + debtData.accruedInterest; //+ debtData.accruedFees;
    }

Through the calcTotalDebt() function, we know that the total debt includes both the principal debt and the interest accrued on the debt. In this CDPVault, the interest accrued on the debt is treated as profit. For example, in the liquidatePosition function, profit is calculated as debtData.accruedInterest, and this profit is treated as interest revenue in other functions as well.

   function liquidatePosition(address owner, uint256 repayAmount) external whenNotPaused {
        //skip ........
        uint256 newDebt;
        uint256 profit;
        uint256 maxRepayment = calcTotalDebt(debtData);
        uint256 newCumulativeIndex;
        if (deltaDebt == maxRepayment) {
            newDebt = 0;
            newCumulativeIndex = debtData.cumulativeIndexNow;
@>>            profit = debtData.accruedInterest;
            position.cumulativeQuotaInterest = 0;
        } 

        //skip ........
    }

Therefore, the loss should only account for the loss of the principal amount. In the liquidatePositionBadDebt function, if repayAmount > debtData.debt, there would actually be a small profit (repayAmount - debtData.debt) instead of a loss. The calculation for the loss should be debtData.debt - repayAmount to correctly reflect the loss of the principal portion.

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the relevant formula for calculating the loss.

Assessed type

Math

[koolexcrypto]

Looks valid. However, this is based on the assumption that loss of revenue is not a loss.
Requesting from the Warden to provide further input to support this assumption. only in PJQA please.

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report

[crypticdefense]

@koolexcrypto I would like to provide further info as requested (my issue #394 is a duplicate). The assumption that the accruedInterest is not a loss is based off how the protocol itself will burn treasury shares to make up for the loss when there is bad debt. Think about it like this, the accruedInterest is the profit the protocol will receive from lending, and if there is bad debt accumulated, that means the accruedInterest has not been paid. Then, when someone liquidates the bad debt position, they can liquidate it for a discount. Since they are paying at a discount, the full debt cannot be repaid, so the protocol will proceed to cover the rest of the amount of debt by burning treasury shares. The loss calculation is as follows: position debt + accruedInterest - repayAmount, where repayAmount is the amount paid by the liquidator. However, the protocol never lost accruedInterest amount, that is just potential profit from lending that was never received.

Even if there exists a case where some of the accruedInterest was paid by the borrower, that is still profit that the protocol is burning. It should only burn the amount of shares equivalent to the debt owed, because that represents the loss. Profit accumulated is not a loss.

So the protocol will proceed to burn loss amount of treasury shares. It is burning extra treasury shares here because it includes accruedInterest, causing a range of issues such as DoS due to insufficient shares and incorrect accounting.

Hope that gives a better understanding of the assumption.

[koolexcrypto]

Thank you for the additional clarification. The issue stays as is.