Borrowers are able to steal the collateral

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-04-lavarage/blob/main/libs/smart-contracts/programs/lavarage/src/processor/liquidate.rs#L107 https://github.com/code-423n4/2024-04-lavarage/blob/main/libs/smart-contracts/programs/lavarage/src/processor/liquidate.rs#L58-L72

Vulnerability details

Impact

Malicious users can submit arbitrary programs during liquidations due to a lack of checks ensuring that the Token program is the real SPL program.

This could potentially allow borrowers to 'liquidate' their own positions, extracting collateral into their own accounts rather than the lender's.

Proof of Concept

Liquidate doesn't have any checks for token_program:

  #[derive(Accounts)]
  pub struct Liquidate<'info> {
      pub mint: Account<'info, Mint>,
      #[account(mut, seeds = [b"position", trader.key().as_ref(), trading_pool.key().as_ref(), random_account_as_id.key().as_ref()],
    bump)]
      pub position_account: Account<'info, Position>,
      #[account(mut)]
      /// CHECK: This is not dangerous because we only use it for its public key
      pub trader: AccountInfo<'info>, // The account of the trader initiating the swap
      #[account(mut)]
      pub trading_pool: Account<'info, Pool>, // The trading pool defines the terms
      #[account(mut, address = get_associated_token_address(&position_account.key(), &mint.key()))]
      pub from_token_account: Account<'info, TokenAccount>,
      #[account(mut, address = get_associated_token_address(&operator.key(), &mint.key()))]
      pub to_token_account: Account<'info, TokenAccount>,
      pub node_wallet: Account<'info, NodeWallet>, // The account of the trader initiating the swap
@>    pub token_program: Program<'info, Token>,
      /// CHECK: We just want the value of this account
      pub random_account_as_id: UncheckedAccount<'info>,
      pub clock: Sysvar<'info, Clock>,
      /// CHECK: Will be checked in code. operator is not required to sign. 
      pub operator: AccountInfo<'info>,
      pub oracle: Signer<'info>,
  }

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

An attacker/borrower might liquidate their own position passing their own program instead of the SPL:

  let seeds = &[
      b"position", 
      trader_key.as_ref(), 
      trading_pool_key.as_ref(),
      random_account_as_id_key.as_ref(),
      &[bump_seed]
  ];
  let signer_seeds = &[&seeds[..]];
  let cpi_accounts = Transfer {
    from: ctx.accounts.from_token_account.to_account_info(),
    to: ctx.accounts.to_token_account.to_account_info(),
    authority: ctx.accounts.position_account.to_account_info(),
  };
  let cpi_ctx = CpiContext::new_with_signer(cpi_program_clone, cpi_accounts, signer_seeds);
  token::transfer(cpi_ctx, ctx.accounts.position_account.collateral_amount)?;

https://github.com/code-423n4/2024-04-lavarage/blob/main/libs/smart-contracts/programs/lavarage/src/processor/liquidate.rs#L58-L72

token::transfer will simply invoke the Attacker's program, but it will be signed by the position:

  pub fn transfer<'info>(
      ctx: CpiContext<'_, '_, '_, 'info, Transfer<'info>>,
      amount: u64,
  ) -> Result<()> {
      let ix = spl_token::instruction::transfer(
          &spl_token::ID,
          ctx.accounts.from.key,
          ctx.accounts.to.key,
          ctx.accounts.authority.key,
          &[],
          amount,
      )?;
      anchor_lang::solana_program::program::invoke_signed(
          &ix,
          &[ctx.accounts.from, ctx.accounts.to, ctx.accounts.authority],
          ctx.signer_seeds,
      )
      .map_err(Into::into)
  }

https://docs.rs/anchor-spl/latest/src/anchor_spl/token.rs.html#11-29

At this point, the attacker's program transfers the collateral to themselves, instead of the lender's account.

Tools Used

Manual review

Recommended Mitigation Steps

Consider checking that the token program is the real SPL program:

  #[derive(Accounts)]
  pub struct Liquidate<'info> {
      pub mint: Account<'info, Mint>,
      #[account(mut, seeds = [b"position", trader.key().as_ref(), trading_pool.key().as_ref(), random_account_as_id.key().as_ref()],
    bump)]
      pub position_account: Account<'info, Position>,
      #[account(mut)]
      /// CHECK: This is not dangerous because we only use it for its public key
      pub trader: AccountInfo<'info>, // The account of the trader initiating the swap
      #[account(mut)]
      pub trading_pool: Account<'info, Pool>, // The trading pool defines the terms
      #[account(mut, address = get_associated_token_address(&position_account.key(), &mint.key()))]
      pub from_token_account: Account<'info, TokenAccount>,
      #[account(mut, address = get_associated_token_address(&operator.key(), &mint.key()))]
      pub to_token_account: Account<'info, TokenAccount>,
      pub node_wallet: Account<'info, NodeWallet>, // The account of the trader initiating the swap
+     #[account(address = anchor_spl::token::ID)]
      pub token_program: Program<'info, Token>,
      /// CHECK: We just want the value of this account
      pub random_account_as_id: UncheckedAccount<'info>,
      pub clock: Sysvar<'info, Clock>,
      /// CHECK: Will be checked in code. operator is not required to sign. 
      pub operator: AccountInfo<'info>,
      pub oracle: Signer<'info>,
  }

Assessed type

Invalid Validation

[c4-judge]

alcueca marked the issue as duplicate of #5

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[c4-sponsor]

piske-alex (sponsor) confirmed

[DadeKuma]

Other dups are separate issues, they are talking about node_wallet, not token_program, this is totally different. Please re-check.

[koolexcrypto]

Hi @DadeKuma

Thank you for your efforts.

Here is my input on this. Unfortunately, this is an invalid issue due to two reasons: First:

Malicious users can submit arbitrary programs during liquidations due to a lack of checks ensuring that the Token program is the real SPL program

The TX for Liquidate function has to be signed by Oracle. Any change in any data will result in an invalid TX in Solana. Furthermore, Oracle is a trusted role. Check README here

Second: The program token id is implicitly checked by solana-program-library when using spl-token CPIs, So developers don't need to verify it anymore. Check this: Fix potential vulnerabilities in programs using spl-token CPIs by adding program id checks

Note: this was a common vulnerability in Solana prior to this fix.

CC: @c4-judge

[DadeKuma]

Hi @koolexcrypto, thanks for the input, TIL. I still have some doubts about this as there is a check in another part of the code but it's missing here, maybe the sponsor can give us further feedback about it as they confirmed the issue.

Regarding the first point, other dups also assume that the Oracle only checks the price/position_size and not the other data before signing (a misuse of the trusted role would be to assume an invalid price, which is what the Oracle is checking).

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid

[alcueca]

The program token id is now checked by default, as @koolexcrypto says.