c4-bot-2 commented 4 months ago

Lines of code

https://github.com/revert-finance/lend/blob/audit/src/utils/FlashloanLiquidator.sol#L43-L45

Vulnerability details

Impact

Users cannot liquidate positions in case liquidationCost = 0.

Proof of concept

The contract FlashloanLiquidator is used by users who want to flash loan for tokens in order to liquidate loan positions. The function FlashloanLiquidator.liquidate will first checks and reverts if liquidationCost = 0:

function liquidate(LiquidateParams calldata params) external {
        (,,, uint256 liquidationCost,) = params.vault.loanInfo(params.tokenId);
        if (liquidationCost == 0) {
            revert NotLiquidatable();
        }
}

The assumption that if liquidationCost =0 then the position is not liquidatable is not correct, since in function V3Vault._calculateLiquidation, this value can be zero of fullValue <= penalty or fullValue <= 10% debt:

uint256 penalty = debt * MAX_LIQUIDATION_PENALTY_X32 / Q32;

            // if value is enough to pay penalty
            if (fullValue > penalty) {
                liquidatorCost = fullValue - penalty;
            } else {
                // this extreme case leads to free liquidation
                liquidatorCost = 0;
            }

I understand that if liquidationCost is 0 then there is no need to flash loan and user can just call V3Vault.liquidate directly. However, most normal users will never calculate the liquidatorCost themselves; they will rely on FlashloanLiquidator and expect that even in cases where liquidatorCost is 0, they can still liquidate the positions. Otherwise it will make them unable to liquidate positions before others.

Recommended mitigation

I think Revert Lend should also returns the isHealthy variable in function loanInfo, this will help distinguish between 2 cases both having liquidationCost=0; one is that the position is indeed not healthy and liquidationCost = 0, the other is that the position is Healthy.

Then, in FlashloanLiquidator, if the position is liquidatable and liquidationCost = 0, simply call liquidate for the user without taking flash loan.

Assessed type

Invalid Validation

c4-sponsor commented 4 months ago

kalinbas marked the issue as disagree with severity

kalinbas commented 4 months ago

FlashloanLiquidator is not needed when liquidationCost = 0. It can be just liquidated directly.

c4-sponsor commented 4 months ago

kalinbas (sponsor) acknowledged

ktg9 commented 4 months ago

Hi @kalinbas , thank you for your response. As I stated in the finding, I understand that if liquidationCost is 0 then there is no need to flash loan and user can just call V3Vault.liquidate directly. However, most users will not calculate the liquidationCost themselves and they can't anticipate the value at the time they call liquidate on FlashloanLiquidator. If FlashloanLiquidator reverts then the users will loose an opportunity to liquidate before others since it might be too late when they realize their transaction reverts and call liquidate on Vault again.

Can you check?

jhsagd76 commented 4 months ago

I agree with the sponsor's viewpoint. In fact, I believe that the current implementation better conforms to the code standards (just IMO). The clearer the boundaries of specific functionalities within a fully functional system, the fewer vulnerabilities there will be under edge conditions.

c4-judge commented 4 months ago

jhsagd76 changed the severity to QA (Quality Assurance)