code-423n4 / 2024-04-lavarage-findings

2 stars 2 forks source link

The lender could force a position to be liquidated by blocking adding extra amount to the collateral #22

Open c4-bot-7 opened 7 months ago

c4-bot-7 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-04-lavarage/blob/main/libs/smart-contracts/programs/lavarage/src/processor/lending.rs#L22-L36

Vulnerability details

Impact

The lender could force a position to be liquidated by preventing the borrower from adding additional collateral amount.

Proof of Concept

When a borrower receives SOL, the collateral has to be added in the same transaction. However, the borrower can maintain their position to protect it from being liquidated (when there is a price movement of the token) by adding additional collateral later. This can be done by calling add_collateral after transferring the tokens to the position account. Then add_collateral function will update ctx.accounts.position_account.collateral_amount accordingly.

    ctx.accounts.position_account.collateral_amount = amt;

swap.rs#L104

The issue is, the lender can update trading_pool.max_borrow to too low amount (or even zero) preventing the borrower from adding additional collateral. This is possible because of this condition:

    require!(
      ((amt as u128)
      .checked_mul(ctx.accounts.trading_pool.max_borrow as u128).expect("overflow")
      .checked_div(10_u128.pow(ctx.accounts.mint.decimals as u32))).expect("overflow") as u64
       >= ctx.accounts.position_account.amount, 
      FlashFillError::ExpectedCollateralNotEnough);

swap.rs#L98-L104

As a result, the position will be liquidated eventually.

Tools Used

Manual analysis

Recommended Mitigation Steps

On borrow, store max borrow on position level and use it for the validation on add collateral. This way, any change to max borrow will apply to new borrowings only.

Assessed type

DoS

c4-sponsor commented 7 months ago

piske-alex (sponsor) acknowledged

c4-sponsor commented 7 months ago

piske-alex (sponsor) confirmed

c4-judge commented 7 months ago

alcueca marked the issue as satisfactory

c4-judge commented 7 months ago

alcueca marked the issue as selected for report

DadeKuma commented 7 months ago

It seems unfair to consider this issue valid as Docs explicitly says that adding collateral to an existing position is not supported yet:

Can I add to my open position? Not at the moment. We have plans to add this feature later but as of now, you can open as many new isolated positions as you like for the same token.

So this is a misuse of the protocol and implies a user error. Please note that these docs were included in the README.

koolexcrypto commented 7 months ago

Hi @DadeKuma

Thank you for the feedback.

I think the docs don't really reflect 100% the actual implementation. Here is an example:

There are 3 ways an open leverage position can be closed.

  1. Repay The trader can repay an open position by sending the amount equivalent to the outstanding loan, i.e. borrowed amount plus interest, and the collateral tokens will be sent to the trader’s wallet.
  2. Sell The trader can initiate to sell the collateral tokens given that the value of the collateral is greater than or equal to the outstanding loan. After the tokens are sold, the proceeds are used first to repay the lender for the outstanding loan. The remaining amount, if any, is returned to the trader’s wallet.
  3. Liquidate When the LTV of the position is equal to or greater than the liquidation LTV (see key parameters), the lender can liquidate the position and the collateral tokens are sent to the lender’s wallet.

There are 3 ways an open leverage position can be closed

alcueca commented 7 months ago

The README must be taken at face value. That means that the mere existence of the add_collateral function is a vulnerability in itself, because users are not expected to be able to add collateral.

The vulnerability described here would not impact legit users (that don't know about add_collateral). It remains as QA to draw attention from the sponsor.

c4-judge commented 7 months ago

alcueca changed the severity to QA (Quality Assurance)

c4-judge commented 7 months ago

alcueca marked the issue as grade-a

c4-judge commented 7 months ago

alcueca marked the issue as not selected for report