zarkk01 - liquidationProtocolFeeAmount are not subtracted from the collaterals of borrower during the liquidation process.

[sherlock-admin2]

zarkk01

High

liquidationProtocolFeeAmount are not subtracted from the collaterals of borrower during the liquidation process.

Summary

Liquidation process does not consider liquidationProtocolFeeAmount as burned collateral from the borrower, leaving this amount on his PositionBalance.

Vulnerability Detail

During a liquidation, the liquidator seizes an amount of the borrower's collaterals while a percentage of them goes to the protocol as liquidationFee. However, during the _burnCollateralTokens() call of LiquidationLogic, the liquidationProtocolFeeAmount, which represents the portion of the collaterals that will go to the protocol, is not burned from the collaterals of the borrower. We can the implementation here :

  function _burnCollateralTokens(
    DataTypes.ReserveData storage collateralReserve,
    DataTypes.ExecuteLiquidationCallParams memory params,
    LiquidationCallLocalVars memory vars,
    DataTypes.PositionBalance storage balances,
    DataTypes.ReserveSupplies storage totalSupplies
  ) internal {
    DataTypes.ReserveCache memory collateralReserveCache = collateralReserve.cache(totalSupplies);
    collateralReserve.updateState(params.reserveFactor, collateralReserveCache);
    collateralReserve.updateInterestRates(
      totalSupplies,
      collateralReserveCache,
      params.collateralAsset,
      IPool(params.pool).getReserveFactor(),
      0,
@>      vars.actualCollateralToLiquidate,
      params.position,
      params.data.interestRateData
    );

    // Burn the equivalent amount of aToken, sending the underlying to the liquidator
@>    balances.withdrawCollateral(totalSupplies, vars.actualCollateralToLiquidate, collateralReserveCache.nextLiquidityIndex);
    IERC20(params.collateralAsset).safeTransfer(msg.sender, vars.actualCollateralToLiquidate);
  }

Link to code

As we can see, the only amount that will be subtracted from the collaterals of the borrower and will deflate his supplyShares is the vars.actualCollateralToLiquidate which is the liquidator's portion. When is the time to handle the liquidationProtocolFeeAmount, we can see that it just performs the safe transfer out without updating neither the colaterals of the borrower nor the underlyingBalance of the reserve and the interest rates. We can see this implementation here :

  function executeLiquidationCall(
    mapping(address => DataTypes.ReserveData) storage reservesData,
    mapping(uint256 => address) storage reservesList,
    mapping(address => mapping(bytes32 => DataTypes.PositionBalance)) storage balances,
    mapping(address => DataTypes.ReserveSupplies) storage totalSupplies,
    mapping(bytes32 => DataTypes.UserConfigurationMap) storage usersConfig,
    DataTypes.ExecuteLiquidationCallParams memory params
  ) external
    // ...

    _burnCollateralTokens(
      collateralReserve, params, vars, balances[params.collateralAsset][params.position], totalSupplies[params.collateralAsset]
    );

    // Transfer fee to treasury if it is non-zero
    if (vars.liquidationProtocolFeeAmount != 0) {
      uint256 liquidityIndex = collateralReserve.getNormalizedIncome();
      uint256 scaledDownLiquidationProtocolFee = vars.liquidationProtocolFeeAmount.rayDiv(liquidityIndex);
      uint256 scaledDownUserBalance = balances[params.collateralAsset][params.position].supplyShares;

      if (scaledDownLiquidationProtocolFee > scaledDownUserBalance) {
        vars.liquidationProtocolFeeAmount = scaledDownUserBalance.rayMul(liquidityIndex);
      }

@>    IERC20(params.collateralAsset).safeTransfer(IPool(params.pool).factory().treasury(), vars.liquidationProtocolFeeAmount);
    }

    // ...

Link to code

Impact

Since the liquidationProtocolFeeAmount is not subtracted from the borrower's collaterals during the liquidation process, the borrower's PositionBalance remains artificially inflated. This discrepancy may cause inconsistencies in accounting, potentially allowing the borrower to access more liquidity or reducing the effectiveness of future liquidations. Moreover, it could lead to incorrect protocol state and affect the calculation of other parameters like interest rates or underlyingBalance of the reserve.

Code Snippet

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/LiquidationLogic.sol#L94
https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/LiquidationLogic.sol#L207

Tool used

Manual Review

Recommendation

Consider implementing this change so to assure that the calculations are right during the liquidation process :

  function _burnCollateralTokens(
    DataTypes.ReserveData storage collateralReserve,
    DataTypes.ExecuteLiquidationCallParams memory params,
    LiquidationCallLocalVars memory vars,
    DataTypes.PositionBalance storage balances,
    DataTypes.ReserveSupplies storage totalSupplies
  ) internal {
    DataTypes.ReserveCache memory collateralReserveCache = collateralReserve.cache(totalSupplies);
    collateralReserve.updateState(params.reserveFactor, collateralReserveCache);
    collateralReserve.updateInterestRates(
      totalSupplies,
      collateralReserveCache,
      params.collateralAsset,
      IPool(params.pool).getReserveFactor(),
      0,
-      vars.actualCollateralToLiquidate,
+      vars.actualCollateralToLiquidate + vars.liquidationProtocolFeeAmount
      params.position,
      params.data.interestRateData
    );

    // Burn the equivalent amount of aToken, sending the underlying to the liquidator
-    balances.withdrawCollateral(totalSupplies, vars.actualCollateralToLiquidate, collateralReserveCache.nextLiquidityIndex);
+    balances.withdrawCollateral(totalSupplies, vars.actualCollateralToLiquidate + vars.liquidationProtocolFeeAmount, collateralReserveCache.nextLiquidityIndex);
    IERC20(params.collateralAsset).safeTransfer(msg.sender, vars.actualCollateralToLiquidate);
  }

Duplicate of #228