LiquidateWithReplacement does not charge swap fees on the borrower

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/LiquidateWithReplacement.sol#L146

Vulnerability details

Impact

When a user places a credit sell limit order, it may be either 1) bought by other users, 2) bought by the LiquidateWithReplacement action. However, for case 2), it does not charge a swap fee for the borrower, which contradicts to the swap fee definition.

Bug Description

Let's first see how LiquidateWithReplacement works. There are two steps:

1. Liquidation: Liquidator sends the amount of debt debt.futureValue to the protocol, and receive collateral tokens as liquidation reward.
2. Buy credit: Use the original lender's debt.futureValue as credit to buy credit from the borrower. This way the original lender can still get the same amount of credit at the same dueDate. The liquidator can then take away the difference between debt.futureValue and the amount sent to the borrower.

Note that step 2 is the same as buying credit using the BuyCreditMarket function.

However, according to the docs, all cash and credit swaps should charge swap fees on the borrower. The issue here is the swap in LiquidateWithReplacement does not charge swap fees.

Protocol fee; charged to the recipient of USDC on cash->credit and credit-> cash swaps. Prorated based on credit tenor (the fee is annualized).

    function executeLiquidateWithReplacement(State storage state, LiquidateWithReplacementParams calldata params)
        external
        returns (uint256 issuanceValue, uint256 liquidatorProfitCollateralToken, uint256 liquidatorProfitBorrowToken)
    {
        emit Events.LiquidateWithReplacement(params.debtPositionId, params.borrower, params.minimumCollateralProfit);

        DebtPosition storage debtPosition = state.getDebtPosition(params.debtPositionId);
        DebtPosition memory debtPositionCopy = debtPosition;
        BorrowOffer storage borrowOffer = state.data.users[params.borrower].borrowOffer;
        uint256 tenor = debtPositionCopy.dueDate - block.timestamp;

        liquidatorProfitCollateralToken = state.executeLiquidate(
            LiquidateParams({
                debtPositionId: params.debtPositionId,
                minimumCollateralProfit: params.minimumCollateralProfit
            })
        );

        uint256 ratePerTenor = borrowOffer.getRatePerTenor(
            VariablePoolBorrowRateParams({
                variablePoolBorrowRate: state.oracle.variablePoolBorrowRate,
                variablePoolBorrowRateUpdatedAt: state.oracle.variablePoolBorrowRateUpdatedAt,
                variablePoolBorrowRateStaleRateInterval: state.oracle.variablePoolBorrowRateStaleRateInterval
            }),
            tenor
        );
>       issuanceValue = Math.mulDivDown(debtPositionCopy.futureValue, PERCENT, PERCENT + ratePerTenor);
        liquidatorProfitBorrowToken = debtPositionCopy.futureValue - issuanceValue;

        debtPosition.borrower = params.borrower;
        debtPosition.futureValue = debtPositionCopy.futureValue;
        debtPosition.liquidityIndexAtRepayment = 0;

        emit Events.UpdateDebtPosition(
            params.debtPositionId,
            debtPosition.borrower,
            debtPosition.futureValue,
            debtPosition.liquidityIndexAtRepayment
        );

        state.data.debtToken.mint(params.borrower, debtPosition.futureValue);
>       state.data.borrowAToken.transferFrom(address(this), params.borrower, issuanceValue);
        state.data.borrowAToken.transferFrom(address(this), state.feeConfig.feeRecipient, liquidatorProfitBorrowToken);
    }

Proof of Concept

Say 2 users setup the same sell limit order.

First user's order was bought by a user via BuyCreditMarket, and second user's order was bought by LiquidateWithReplacement, but the swap fee is only charged to the first user. This is unfair and is not mentioned anywhere expected in the documents.

Tools Used

Manual review

Recommended Mitigation Steps

Also charge swap fees during executeLiquidateWithReplacement.

Assessed type

Other

[aviggiano]

This is the intended behavior. The reason why the new borrower does not pay the swap fee is that indeed there is no credit for cash swap. The fee was already paid by the first borrower.

Consider the following scenario:

1. Alice borrows from Bob. Alice pays the swap fee
2. There is a price change and now Alice is liquidatable
3. The keeper bot calls liquidateWithReplacement and replaces Alice by Candy
4. Candy does not pay the swap fee since there is no cash for credit swap. Alice already paid the fee in step 1

[aviggiano]

Feedback from the team:

• Technically valid observation
• Whether we fix it or not is a product decision
• Technically this is a valid one since it is a credit for cash operation, since the chosen borrower sells his credit for cash
• Just imagine what would happen if that credit sell limit order would have been filled by a credit buy market order: the swap fee would have been charged, and liquidateWithReplacement() is essentially not different from doing a credit buy limit order filling a credit sell limit order
• From a product perspective it is not clear if it was decided to treat this in a special way i.e. not to charge the swap fee on the credit seller, as it should be, as a form of incentive since it is highly desirable from the protocol perspective to have many such offers as the protocol earns a lot from a liquidation with replacement
• The liquidateWithReplacement() leads to the some outcome of the original loan for the lender and it should be indistinguishable from a standard credit sell limit order being filled for the borrower
• So regarding the latter, for consistency, the borrower should pay the swap fee, however, it is fair to observe the protocol already makes a big profit from liquidateWithReplacement() since it takes the full time value of money that otherwise in case of a standard liquidation would have been earned entirely by the lender, and since atm liquidateWithReplacement() atm has some limitations (see the auditors remarks about the problem of running liquidateWithReplacement() on large loans, due to the inability of fractionalize debt) then it could make sense to incentivize it
• I say "could" because that's a product decision, not relevant for the protocol accounting

[hansfriese]

Thanks for the detailed feedback. Will keep as a valid Medium.

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report