liquidations close borrow positions but lender's stroage variable are not updated correctly.

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

liquidations via liquidate() return/add funds to the lender's token account (funds was taken or lent out during the borrow to user) and close borrow positions but dont reduce the total amount borrowed. This means:

• on the next borrow (via call to swap.rs borrow() fcn) from the lender's node wallet, program will add to the already false/inflated/not reduced node_wallet_total_borrow amount thus causing accouting issues/false data to persist/updated into contract storage.

• ui will display wrong data as node_wallet.total_Borrowed will be higher than it ought to be

Proof of Concept

the function liquidate() does not and decrease the ctx.accounts.node_wallet.total_borrowed after a liquidation is executed.

If during borrow() these variables are modified to reflect a borrow action as seen here then they should be modified again to reflect a liquidation event as liquidations return the borrwed funds back to the lender and should reduce the total_borrowed amount because funds are not in a borrow position any longer. ctx.accounts.node_wallet.total_borrowed tracks the total amount of lender funds currently lent/borrowed to users in various positions at any given moment.

Since this modifications is not made,this means ctx.accounts.node_wallet.total_borrowed will not be decreased on liquidation thus falsly reporting that the lender has higher amount of funds in active borrow position(s). Upon another borrow by the lender to a user the ctx.accounts.node_wallet.total_borrowed will be updated/increased in storage to be much higher than actual.

Scenario

• lender funds node-wallet with 1 sol, ctx.accounts.node_wallet.total_funds is increased to 1
• user borrows 0.5 sol from lender. ctx.accounts.node_wallet.total_funds is decreased to 0.5 sol and ctx.accounts.node_wallet.total_borrowed is increased to 0.5 sol
• user gets liquidated and borrow position is closed. 0.5 sol is sent back to lender
• ctx.accounts.node_wallet.total_borrowed is still old amount borrowed out by lender before the liquidation (0.5 sol). even though lender actually has no funds borrowed out at the moment. (all collateral has been returned)
• user borrows another 0.5 sol loan from lender's node wallet, ctx.accounts.node_wallet.total_borrowed becomes 1 sol. even though at that moment lender only supplied one loan position worth 0.5 sol.

I made a test to further describe this scenario. To run the test please

1.) modify the PositionOpenEvent and PositionCloseEvent in state/position.rs to be like below. I made the modification to be able to watch the changes to these values (total_funds and total_borrowed) better in the tests.

#[event]
pub struct PositionCloseEvent {
  pub pool: Pubkey,

  pub amount: u64,

  pub user_paid: u64,

  pub collateral_amount: u64,

  pub open_timestamp: i64,
  
  pub trader: Pubkey,

  pub close_type: u8,

  pub close_timestamp: i64,

  pub closing_position_size: u64,

  pub node_wallet_total_borrowed: u64,
  
  pub node_wallet_total_funds: u64,
}

#[event]
pub struct PositionOpenEvent {
  pub pool: Pubkey,

  pub amount: u64,

  pub user_paid: u64,

  pub collateral_amount: u64,

  pub open_timestamp: i64,
  
  pub trader: Pubkey,

  pub node_wallet_total_borrowed: u64,

  pub node_wallet_total_funds: u64,

}

2.) modify the PositionOpenEvent emit in add_collateral() fcn in swap.rs to be

    emit!(PositionOpenEvent {
      pool: ctx.accounts.position_account.pool,
      amount: ctx.accounts.position_account.amount,
      user_paid: ctx.accounts.position_account.user_paid,
      collateral_amount: ctx.accounts.position_account.collateral_amount,
      open_timestamp: ctx.accounts.position_account.timestamp,
      trader: ctx.accounts.position_account.trader,
      node_wallet_total_borrowed: ctx.accounts.node_wallet.total_borrowed,
      node_wallet_total_funds: ctx.accounts.node_wallet.total_funds,
    });

2.) modify the PositionCloseEvent emit in repay_sol() fcn in swapback.rs to be

  emit!(PositionCloseEvent {
    pool: ctx.accounts.position_account.pool,
    amount: ctx.accounts.position_account.amount,
    user_paid: ctx.accounts.position_account.user_paid,
    collateral_amount: ctx.accounts.position_account.collateral_amount,
    open_timestamp: ctx.accounts.position_account.timestamp,
    trader: ctx.accounts.position_account.trader,
    close_type: PositionCloseType::ClosedByUser as u8,
    close_timestamp: ctx.accounts.clock.unix_timestamp,
    closing_position_size: closing_position_size,
    node_wallet_total_borrowed:  ctx.accounts.node_wallet.total_borrowed,
    node_wallet_total_funds: ctx.accounts.node_wallet.total_funds
  });

3.) modify the PositionCloseEvent emit in liquidate() fcn in liquidate.rs to be

    emit!(PositionCloseEvent {
      pool: ctx.accounts.position_account.pool,
      amount: ctx.accounts.position_account.amount,
      user_paid: ctx.accounts.position_account.user_paid,
      collateral_amount: ctx.accounts.position_account.collateral_amount,
      open_timestamp: ctx.accounts.position_account.timestamp,
      trader: ctx.accounts.position_account.trader,
      close_type: PositionCloseType::Liquidated as u8,
      close_timestamp: ctx.accounts.clock.unix_timestamp,
      closing_position_size: position_size,
      node_wallet_total_borrowed: ctx.accounts.node_wallet.total_borrowed,
      node_wallet_total_funds: ctx.accounts.node_wallet.total_funds
    });

4.) modify the AddCollateral context in add_collateral.rs like below

#[derive(Accounts)]
pub struct AddCollateral<'info> {
  #[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)]
  pub trader: Signer<'info>, // The account of the trader initiating the swap
  #[account(mut)]
  pub trading_pool: Account<'info, Pool>, // The trading pool defines the terms
  pub system_program: Program<'info, System>,
  pub mint: Account<'info, Mint>,
  #[account(address = get_associated_token_address(&position_account.key(), &mint.key()))]
  pub to_token_account: Account<'info, TokenAccount>,
  /// CHECK: We just want the value of this account
  pub random_account_as_id: UncheckedAccount<'info>,

  pub node_wallet: Account<'info, NodeWallet>, //included so i can track the changes to node_wallet.total_borrwed and node_wallet.total_funds 
}

5.) paste in ./tests folder and run with ORACLE_PUB_KEY=ATeSYS4MQUs2d6UQbBvs9oSNvrmNPU1ibnS2Dmk21BKZ anchor test

import * as anchor from '@coral-xyz/anchor';
import {
  Keypair,
  PublicKey,
  Signer,
  SystemProgram,
  SYSVAR_CLOCK_PUBKEY,
  SYSVAR_INSTRUCTIONS_PUBKEY,
  Transaction,
} from '@solana/web3.js';

import { Lavarage } from '../target/types/lavarage';

import {
  createMint,
  createTransferCheckedInstruction,
  getAccount,
  getOrCreateAssociatedTokenAccount,
  mintTo,
  TOKEN_PROGRAM_ID,
} from '@solana/spl-token';
import { web3 } from '@coral-xyz/anchor';
export function getPDA(programId, seed) {
  const seedsBuffer = Array.isArray(seed) ? seed : [seed];

  return web3.PublicKey.findProgramAddressSync(seedsBuffer, programId)[0];
}

let nodeWalletTotalBorrowedDuringALoanCollection;
let nodeWalletTotalFundsDuringALoanCollection;
describe('lavarage', () => {
  anchor.setProvider(anchor.AnchorProvider.env());
  const program: anchor.Program<Lavarage> = anchor.workspace.Lavarage;
  const nodeWallet = anchor.web3.Keypair.generate();
  const anotherPerson = anchor.web3.Keypair.generate();
  const seed = anchor.web3.Keypair.generate();
  // TEST ONLY!!! DO NOT USE!!!
  const oracleKeyPair = anchor.web3.Keypair.fromSecretKey(
    Uint8Array.from([
      70, 207, 196, 18, 254, 123, 0, 205, 199, 137, 184, 9, 156, 224, 62, 74,
      209, 0, 80, 73, 146, 151, 175, 68, 182, 180, 53, 91, 214, 7, 167, 209,
      140, 140, 158, 10, 59, 141, 76, 114, 109, 208, 44, 110, 77, 64, 149, 121,
      7, 226, 125, 0, 105, 29, 76, 131, 99, 95, 123, 206, 81, 5, 198, 140,
    ]),
  );
  let tokenMint;
  let userTokenAccount;

  const provider = anchor.getProvider();

  async function mintMockTokens(
    people: Signer,
    provider: anchor.Provider,
    amount: number,
  ): Promise<any> {
    const connection = provider.connection;

    const signature = await connection.requestAirdrop(
      people.publicKey,
      2000000000,
    );
    await connection.confirmTransaction(signature, 'confirmed');

    // Create a new mint
    const mint = await createMint(
      connection,
      people,
      people.publicKey,
      null,
      9, // Assuming a decimal place of 9
    );

    // Get or create an associated token account for the recipient
    const recipientTokenAccount = await getOrCreateAssociatedTokenAccount(
      connection,
      people,
      mint,
      provider.publicKey,
    );

    // Mint new tokens to the recipient's token account
    await mintTo(
      connection,
      people,
      mint,
      recipientTokenAccount.address,
      people,
      amount,
    );

    return {
      mint,
      recipientTokenAccount,
    };
  }

  // Setup phase
  it('Should mint new token!', async () => {
    const { mint, recipientTokenAccount } = await mintMockTokens(
      anotherPerson,
      provider,
      200000000000,
    );
    tokenMint = mint;
    userTokenAccount = recipientTokenAccount;
  }, 20000);
  it('Should create lpOperator node wallet', async () => {
    await program.methods
      .lpOperatorCreateNodeWallet()
      .accounts({
        nodeWallet: nodeWallet.publicKey,
        systemProgram: anchor.web3.SystemProgram.programId,
        operator: program.provider.publicKey,
      })
      .signers([nodeWallet])
      .rpc();
  });

  it('Should create trading pool', async () => {
    const tradingPool = getPDA(program.programId, [
      Buffer.from('trading_pool'),
      provider.publicKey.toBuffer(),
      tokenMint.toBuffer(),
    ]);
    await program.methods
      .lpOperatorCreateTradingPool(50)
      .accounts({
        nodeWallet: nodeWallet.publicKey,
        systemProgram: anchor.web3.SystemProgram.programId,
        operator: program.provider.publicKey,
        tradingPool,
        mint: tokenMint,
      })
      .rpc();
  });

  it('Should fund node wallet', async () => {
    await program.methods
      .lpOperatorFundNodeWallet(new anchor.BN(5000000000))
      .accounts({
        nodeWallet: nodeWallet.publicKey,
        systemProgram: anchor.web3.SystemProgram.programId,
        funder: program.provider.publicKey,
      })
      .rpc();
  });

  it('Should set maxBorrow', async () => {
    const tradingPool = getPDA(program.programId, [
      Buffer.from('trading_pool'),
      provider.publicKey.toBuffer(),
      tokenMint.toBuffer(),
    ]);
    // X lamports per 1 Token
    await program.methods
      .lpOperatorUpdateMaxBorrow(new anchor.BN(50))
      .accountsStrict({
        tradingPool,
        nodeWallet: nodeWallet.publicKey,
        operator: provider.publicKey,
        systemProgram: anchor.web3.SystemProgram.programId,
      })
      .rpc();
  });

  // borrow
  it('Should borrow SOL', async () => {
    const tradingPool = getPDA(program.programId, [
      Buffer.from('trading_pool'),
      provider.publicKey.toBuffer(),
      tokenMint.toBuffer(),
    ]);
    // create ATA for position account
    const positionAccount = getPDA(program.programId, [
      Buffer.from('position'),
      provider.publicKey?.toBuffer(),
      tradingPool.toBuffer(),
      // unique identifier for the position
      seed.publicKey.toBuffer(),
    ]);
    const positionATA = await getOrCreateAssociatedTokenAccount(
      provider.connection,
      anotherPerson,
      tokenMint,
      positionAccount,
      true,
    );
    // actual borrow
    const borrowIx = await program.methods
      .tradingOpenBorrow(new anchor.BN(10000), new anchor.BN(5000))
      .accountsStrict({
        positionAccount,
        trader: provider.publicKey,
        tradingPool,
        nodeWallet: nodeWallet.publicKey,
        randomAccountAsId: seed.publicKey,
        // frontend fee receiver. could be any address. opening fee 0.5%
        feeReceipient: anotherPerson.publicKey,
        systemProgram: anchor.web3.SystemProgram.programId,
        clock: anchor.web3.SYSVAR_CLOCK_PUBKEY,
        instructions: anchor.web3.SYSVAR_INSTRUCTIONS_PUBKEY,
      })
      .instruction();
    const transferIx = createTransferCheckedInstruction(
      userTokenAccount.address,
      tokenMint,
      positionATA.address,
      provider.publicKey,
      100000000000,
      9,
    );

    // i modified the PositionOpenEvent struct to read/monitor the values better
    const positionOpenEventListener = program.addEventListener(
      'PositionOpenEvent',
      (event) => {
        nodeWalletTotalBorrowedDuringALoanCollection =
          event.nodeWalletTotalBorrowed;
        nodeWalletTotalFundsDuringALoanCollection = event.nodeWalletTotalFunds;
        console.log(
          'node wallet total Borrowed at loan collection time =',
          nodeWalletTotalBorrowedDuringALoanCollection,
        );

        console.log(
          'node wallet total funds at loan collection time =',
          nodeWalletTotalFundsDuringALoanCollection,
        );
      },
    );

    // the param in this method is deprecated. should be removed.
    const addCollateralIx = await program.methods
      .tradingOpenAddCollateral()
      .accountsStrict({
        positionAccount,
        tradingPool,
        systemProgram: anchor.web3.SystemProgram.programId,
        trader: provider.publicKey,
        randomAccountAsId: seed.publicKey,
        mint: tokenMint,
        toTokenAccount: positionATA.address,
        nodeWallet: nodeWallet.publicKey,
      })
      .instruction();

    const tx = new Transaction()
      .add(borrowIx)
      .add(transferIx)
      .add(addCollateralIx);
    await provider.sendAll([{ tx }]);

    program.removeEventListener(positionOpenEventListener);
  });

  it('Should liquidate but total_borrow not reduced/updated', async () => {
    const tradingPool = getPDA(program.programId, [
      Buffer.from('trading_pool'),
      provider.publicKey.toBuffer(),
      tokenMint.toBuffer(),
    ]);
    const positionAccount = getPDA(program.programId, [
      Buffer.from('position'),
      provider.publicKey?.toBuffer(),
      tradingPool.toBuffer(),
      // unique identifier for the position
      seed.publicKey.toBuffer(),
    ]);
    const positionATA = await getOrCreateAssociatedTokenAccount(
      provider.connection,
      anotherPerson,
      tokenMint,
      positionAccount,
      true,
    );
    let totalBorrowedByNodeWlletAtLiquidationTime;
    let totalFundsInNodeWlletAtLiquidationTime;

    // i modified the PositionCloseEvent struct to read/monitor the values better

    const positionCloseEventListener = program.addEventListener(
      'PositionCloseEvent',
      (event) => {
        totalBorrowedByNodeWlletAtLiquidationTime =
          event.nodeWalletTotalBorrowed;
        totalFundsInNodeWlletAtLiquidationTime = event.nodeWalletTotalFunds;
        console.log(
          'total node bal at this time =',
          totalBorrowedByNodeWlletAtLiquidationTime,
        );
      },
    );

    await program.methods
      .lpLiquidate(new anchor.BN(5501))
      .accountsStrict({
        mint: tokenMint,
        oracle: oracleKeyPair.publicKey,
        positionAccount,
        trader: provider.publicKey,
        tradingPool,
        fromTokenAccount: positionATA.address,
        toTokenAccount: userTokenAccount.address,
        operator: provider.publicKey,
        nodeWallet: nodeWallet.publicKey,
        tokenProgram: TOKEN_PROGRAM_ID,
        randomAccountAsId: seed.publicKey,
        clock: SYSVAR_CLOCK_PUBKEY,
      })
      .signers([oracleKeyPair])
      .rpc();

    /** because user has been liquidated, the loan taken from the lender's nodewallet hasd been repaid,
        hence the node_wallet_total_borrowed should be reduced to show that no more funds are in borrowed positions
       but this is not the case as lender node_wallet.totalBorrowed before and after borrower liquidation of a loan position is the same */
    expect(totalBorrowedByNodeWlletAtLiquidationTime).toEqual(
      nodeWalletTotalBorrowedDuringALoanCollection,
    );

    /** so because of this  there is no change in value to node_wallet_total_borrowed. upon new borrow, the false/inflated node_wallet_total_borrowed 
        will be further increased in storage thus causing bad accounting in storage.  */

    program.removeEventListener(positionCloseEventListener);
  });
});

Tools Used

anchor, manual review

Recommended Mitigation Steps

at the end current liquidate() fcn logic, add a line to reduce the total_borrwed by the amount liquidated.

let borrowedAmountLiquidated = ctx.accounts.position_account.amount
  ctx.accounts.node_wallet.total_borrowed -= borrowedAmountLiquidated;

Assessed type

Context

[c4-judge]

alcueca marked the issue as primary issue

[c4-sponsor]

piske-alex (sponsor) confirmed

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[DadeKuma]

This seems QA to me as:

1. total_borrowed isn't used anywhere and its actual value has zero impact on the protocol
2. This is false:

will be updated/increased in storage to be much higher than actual

The storage used will be the same, no matter the value of total_borrowed, and there isn't any increase in cost no matter its value.

[adeolu98]

1. total_borrowed isn't used anywhere and its actual value has zero impact on the protocol

Total borrowed is always used by the borrow fcn for every node wallet every time there is a new borrow or a loan repay. the values are increased or decreased respectively.

This is false: will be updated/increased in storage to be much higher than actual. The storage used will be the same, no matter the value of total_borrowed, and there isn't any increase in cost no matter its value.

why do you say this @DadeKuma ? anyway i wrote another extensive test (similar to the one in the issue) for it. Upon second borrow from the lender's node wallet, totalBorrowed will be increased again to become further higher than actual if first borrow is already liquidated. check it here -> https://gist.github.com/adeolu98/fd2ac477e95a0c02fdc2b74523fa3cab
Perhaps you talk about storage cost? this issue is not about that. this is about the value written into storage/global variable ctx.accounts.node_wallet.total_borrowed.

this is clearly an accounting issue as total_funds and total_borrowed are used to track user accounting in the protocol. if you say it has zero impact then you are wrong. The protocol clearly manipulates the values of total_funds and total_borrowed on subsequent borrows, repays for loans serviced by a node wallet. This (total_borrowed) can stack up to become a ridiculously much higher value than actual.

[DadeKuma]

Hey @adeolu98

Total borrowed is always used by the borrow fcn for every node wallet every time there is a new borrow or a loan repay. the values are increased or decreased respectively.

Yes, but only to keep track of the amount, it's never actually used for anything:
https://github.com/search?q=repo%3Acode-423n4%2F2024-04-lavarage%20total_borrowed%20&type=code

why do you say this @DadeKuma ? ... Perhaps you talk about storage cost? this issue is not about that. this is about the value written into storage/global variable

I was saying that total_borrowed is a u64 and it always allocates 8 bytes, I thought that you were referring about storage cost.

this is clearly an accounting issue as total_funds and total_borrowed are used to track user accounting in the protocol.

What you are saying is true, this variable can have a wrong value. But I don't see how it can be abused in the current codebase. This is why I think it's QA, funds/availability of the protocol are not at risk.

[adeolu98]

What you are saying is true, this variable can have a wrong value. But I don't see how it can be abused in the current codebase. This is why I think it's QA, funds/availability of the protocol are not at risk.

Quote from c4 docs

Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

but @DadeKuma the protocol doesn't function properly or as intended. It is only tracked/manipulated in borrow, repay and not liquidations. liquidations also affect borrower and lender node_wallet. ctx.accounts.node_wallet.total_borrowed was created for a particular purpose but isn't currently being utilized in all required parts of the code and this causes subsequent borrow and repay to manipulate wrong values and execute sucessfully. Its a miss in logic.

[alcueca]

@DadeKuma is right, total_borrowed is tracked, but not used by the protocol. The function of the protocol is not impacted (i.e., users can do everything that they wanted to do). There is no statement of how value could leak under reasonable assumptions.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[c4-judge]

alcueca marked the issue as not selected for report