Fix: position cannot be liquidated when turn on liquidation protocol fee

[zhoujia6139]

No description provided.

[eboadom]

@zhoujia6139 could you provide some extra context on why you think this doesn't work as expected?
From my perspective it does work fine, but would like to understand your point of view

[zhoujia6139]

let me take an example:
if the user PToken balance(before rebase) is 102, and PToken normalized income index is 1000.75
then userCollateralBalance(the user PToken balance after rebase) will be 102077(102 * 1000.75 = 102076.5 => 102077)

during liquidation, if actualCollateralToLiquidate = 51539
then liquidationProtocolFeeAmount will be 50538 (102077 - 51539 = 50538)

then the PToken balance(before rebase) for actualCollateralToLiquidate will be 52(51539 / 1000.75 = 51.500374718960779 => 52)
the PToken balance(before rebase) for liquidationProtocolFeeAmount will be 51(50538 / 1000.75 = 50.50012490632026 => 51)

so the PToken balance(before rebase) needed for liquidation is 103(52 + 51), and the user PToken balance(before rebase) is only 102, makes the following call fail:

if (vars.liquidationProtocolFeeAmount != 0) {
  vars.collateralAToken.transferOnLiquidation(
    params.user,
    vars.collateralAToken.RESERVE_TREASURY_ADDRESS(),
    vars.liquidationProtocolFeeAmount
  );
}

[eboadom]

@zhoujia6139 not sure why you consider your flow the "before the rebase" state (I assume you refer to data before the liquidity index gets updated).

_calculateAvailableCollateralToLiquidate() uses "after rebase" data, which means fully updated. Both in terms of reserve's data, which was updated on updateState() and user data (e.g. vars.userCollateralBalance using balanceOf() with already updated liquidity index).

Then the _calculateAvailableCollateralToLiquidate() does all the calculations to take into account balances, liquidation bonus, and liquidation protocol fee. So there should never be a situation where the flow coming back to executeLiquidationCall() will try to split (transfer) more funds of what the user has.

Do you have any test for example in mainnet fork showing how in this scenario the code fails?

[zhoujia6139]

I think the test case I added inliquidation-with-fee.spec.tsis very straightforward to illustrate the issue.

[eboadom]

@zhoujia6139 your tests are positive, checking that after your changes the system works, but not checking that it doesn't before. In addition both the naming on your previous message (pToken? I assumed aToken) and amounts don't really fit.

To clarify why the logic is actually correct, the flow of liquidation is the following:

1. Liquidator calls liquidationCall() on the Pool, with the debt amount to cover, debt asset, and collateral asset to receive.
2. Flow goes to executeLiquidationCall().
3. Debt asset data is updated (indexes, accruedToTreasury) via the usual updateState().
4. Debt to cover is evaluated/recalculated via _calculateDebt() depending on HF and close factor.
5. Calculation of total balance of the user at this exact moment "scaled up". This is exactly the maximum that can be potentially liquidated.
6. Flow goes to _calculateAvailableCollateralToLiquidate()
7. From the debt to cover, the equivalent collateral is calculated. Simplifying a bit the explanation, this includes an extra of liquidation bonus, minus the liquidation protocol fee, taken out from the bonus as a percentage of it. So if you the collateral calculated is 2 WETH, bonus 5% and liquidation protocol fee 10%, the final collateral to be received by the liquidator will be (math units apart) 2 * (1 + (0.05 * 0.9)) = 2.09 WETH, liquidation protocol fee will be the 0.01 WETH. So a total of 2.1 WETH will be taken from the user balance. Important to remember that the user has at this point more or equal than 2.1 WETH vs the 2.1 WETH he will lose
8. Flow coming back to executeLiquidationCall()
9. Misc operations not related with this potential issue: burn debt tokens, isolated debt update
10. And now the key point. First, via _burnCollateralATokens() the state of the collateral reserve is updated, so liquidity index grows. Second, the aTokens going to the liquidator are burnt and sent to him. It is important to highlight that for it, the vars.actualCollateralToLiquidate (2.09 in our example) is converted to scaled balance by dividing by the just updated liquidity index. But the collateral of the user was calculated on 5) using the equivalent to this new index, the getNormalizedIncome(), so the amount is scaled down is exact. 2.09 WETH out to the liquidator.
11. Finally in what affects this analysis, the vars.liquidationProtocolFeeAmount (0.01 WETH in our example) is sent to the Collector via transferOnLiquidation(). This amount is scaled down using the getNormalizedIncome(), which is exactly the same as the liquidity index at the moment, because the timestamp stored on the reserve is the current one, block.timestamp

[zhoujia6139]

oh yeah. PToken is actually AToken, it's my fault. You can remove the solidity code I added, then the test case will fail.

The key point of this issue is the WadRayMath library you used. it's a rounded operation(If a value is >=.5, will be rounded up, otherwise rounded down.).

I list the actual failed data for test case I added here.

The user AToken balance(after scale) is 1000000000000000000, and AToken normalized income index is 1000018968212153143659992975
then userCollateralBalance(ie, the user AToken balance before scale) will be 10000189682121531437(1000000000000000000 * 1000018968212153143659992975 / 1000000000000000000000000000 = 10000189682121531436.59992975 => 10000189682121531437)

During liquidation, if actualCollateralToLiquidate = 9976379706687908743
then liquidationProtocolFeeAmount will be 23809975433622694 (10000189682121531437 - 9976379706687908743 = 23809975433622694)

Then the AToken balance(after scale) for actualCollateralToLiquidate part will be 9976190476190476191(9976379706687908743 * 1000000000000000000000000000 / 1000018968212153143659992975 = 9976190476190476190.7729 => 9976190476190476191)
and the AToken balance(after scale) for liquidationProtocolFeeAmount part will be 23809523809523810(23809975433622694 * 1000000000000000000000000000 / 1000018968212153143659992975 = 23809523809523809.6271 => 23809523809523810)

so the AToken balance(after scale) needed for liquidation is 1000000000000000001(9976190476190476191 + 23809523809523810), while the user AToken balance(after scale) is only 1000000000000000000, makes the following call fail:

if (vars.liquidationProtocolFeeAmount != 0) {
  vars.collateralAToken.transferOnLiquidation(
    params.user,
    vars.collateralAToken.RESERVE_TREASURY_ADDRESS(),
    vars.liquidationProtocolFeeAmount
  );
}

[zhoujia6139]

When Integer A multiply by an index which is greater than 1.0 and end up with a rounded up operation to Integer B.

Then we divide B to two parts, Integer B1 and Integer B2.

if we divide B1 by this index and end up with a rounded up operation to Integer C1 and
if we divide B2 by this index and end up with a rounded up operation to Integer C2.

then C1 + C2 == A + 1.

The liquidation logic code assume C1 + C2 == A, so in some case it will fail.

[eboadom]

@zhoujia6139 now I see what you are referring to. And yes, agree that precision can be a problem in some cases.

Regarding the change, looks correct to me, will just do some suggestions about naming

[zhoujia6139]

ok, great.

[miguelmtzinf]

Thanks for your contribution @zhoujia6139 !