Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A2-security - Inaccurate Interest Rate Calculation in Liquidation Process #321

Closed
sherlock-admin4 opened this issue Sep 10, 2024 · 8 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Escalation Resolved This issue's escalations have been approved/rejected Medium A Medium severity issue. Reward A payout will be made for this issue

Comments

@sherlock-admin4
Copy link
Contributor

sherlock-admin4 commented Sep 10, 2024

A2-security

Medium

Inaccurate Interest Rate Calculation in Liquidation Process

Summary

The liquidation process in the LiquidationLogic.sol contract incorrectly calculates the interest rate for the collateral asset by underestimating the amount of liquidity taken. The calculation only considers the amount seized by the liquidator and ignores the additional liquidation fee sent to the protocol treasury. This leads to an inaccurate utilization ratio and consequently affects the liquidity index and the yield for depositors.

Vulnerability Detail

  • The liquidation process in the LiquidationLogic.sol contract involves seizing collateral from the violator and and then update the interest rate since some collateral will be leaving the protocol. The seized amount consists of two parts: the liquidation amount taken by the liquidator and the liquidation fee sent to the protocol treasury.
 >>  (vars.actualCollateralToLiquidate, vars.actualDebtToLiquidate, vars.liquidationProtocolFeeAmount) = _calculateAvailableCollateralToLiquidate(/*prams*/);
  • However, the current implementation of the _burnCollateralTokens function, which is responsible for updating the interest rates, only considers the liquidation amount taken by the liquidator (vars.actualCollateralToLiquidate) when calculating the interest rate. It ignores the liquidation fee (vars.liquidationProtocolFeeAmount) that is also sent to the treasury.

Here's the relevant code snippet from LiquidationLogic.sol:

  function _burnCollateralTokens(
  DataTypes.ReserveData storage collateralReserve,
  DataTypes.ExecuteLiquidationCallParams memory params,
  LiquidationCallLocalVars memory vars,
  DataTypes.PositionBalance storage balances,
  DataTypes.ReserveSupplies storage totalSupplies
  ) internal {
  // ...
  collateralReserve.updateInterestRates(
  totalSupplies,
  collateralReserveCache,
  params.collateralAsset,
  IPool(params.pool).getReserveFactor(),
  0,
  vars.actualCollateralToLiquidate, // @audit-issue: underestimated amount, should include liquidation fee
  params.position,
  params.data.interestRateData
  );
  // ...
  }

The vulnerability lies in the fact that the liquidation fee, which is sent to the treasury, is not accounted for in the interest rate calculation, leading to an underestimation of the liquidity taken and an incorrect intrestRate. (lost of yield for suppliers since utilisationRation will be less than it should , and all intrestRate models are a utilisationRAtion functions).

Note: we know that the interest rate model is out-of-scope for this audit. However, we don't need to know the specific model used by the protocol to identify this issue. The _updateInterestRate function is used correctly in other operations like supply and withdraw. This inconsistency in the liquidation process is sufficient to demonstrate the vulnerability, regardless of the exact interest rate model employed.

Impact

  • The inconsistent handling of liquidation fees leads to incorrect interest rate calculations, resulting in loss of yield for depositors and potential of funds locked in the protocol (which can happen with some interest rate models).

Code Snippet

Tool used

Manual Review

Recommendation

  • To address this issue, the _burnCollateralTokens function should be updated to include the liquidation fee (vars.liquidationProtocolFeeAmount) when calculating the interest rate.
function _burnCollateralTokens(
DataTypes.ReserveData storage collateralReserve,
DataTypes.ExecuteLiquidationCallParams memory params,
LiquidationCallLocalVars memory vars,
DataTypes.PositionBalance storage balances,
DataTypes.ReserveSupplies storage totalSupplies
) internal {
// ...

  collateralReserve.updateInterestRates(
  totalSupplies,
  collateralReserveCache,
  params.collateralAsset,
  IPool(params.pool).getReserveFactor(),
  0,
- vars.actualCollateralToLiquidate,
+ vars.actualCollateralToLiquidate + vars.liquidationProtocolFeeAmount;
  params.position,
  params.data.interestRateData
  );
  // ...
  }

Duplicate of #401

@github-actions github-actions bot added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A High severity issue. labels Sep 20, 2024
@sherlock-admin3 sherlock-admin3 changed the title Clever Ebony Halibut - Inaccurate Interest Rate Calculation in Liquidation Process A2-security - Inaccurate Interest Rate Calculation in Liquidation Process Oct 3, 2024
@sherlock-admin3 sherlock-admin3 added the Reward A payout will be made for this issue label Oct 3, 2024
@elhajin
Copy link

elhajin commented Oct 5, 2024

this is not a duplicate of #228 , we submitted #395 which indeed duplicate of #228 , but this one is different, this is the same as #401 and should be grouped with it.

@obou07
Copy link

obou07 commented Oct 5, 2024

escalate
per comment

@sherlock-admin3
Copy link
Contributor

escalate
per comment

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

@sherlock-admin4 sherlock-admin4 added the Escalated This issue contains a pending escalation label Oct 5, 2024
@Tomiwasa0
Copy link
Collaborator

If i may add also, This issue , issue 341, 257 and 145 should all be duplicated with 401. Root cause is about interest update not liquidity removal.

@cvetanovv
Copy link

I agree with the escalation and @Tomiwasa0 comment.

#321, #341, #257, and #145 are duplicates of #401 because of the same root cause that the interest rate would be calculated wrong if the liquidation fee is not accounted in _burnCollateralTokens().

Planning to accept the escalation and duplicate #321, #341, #257, and #145 with #401.

@WangSecurity WangSecurity added Medium A Medium severity issue. and removed High A High severity issue. labels Oct 21, 2024
@WangSecurity
Copy link

WangSecurity commented Oct 21, 2024

Result:
Medium
Duplicate of #401

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Oct 21, 2024

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin3 sherlock-admin3 removed the Escalated This issue contains a pending escalation label Oct 21, 2024
@sherlock-admin4 sherlock-admin4 added the Escalation Resolved This issue's escalations have been approved/rejected label Oct 21, 2024
@0xSpearmint
Copy link

@cvetanovv Can you please take a look at this comment on 401.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Escalation Resolved This issue's escalations have been approved/rejected Medium A Medium severity issue. Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

9 participants