Trendy Brick Stallion - Incorrect check in `claim_fee.rs::claim_handler`

[sherlock-admin4]

Trendy Brick Stallion

Medium

Incorrect check in claim_fee.rs::claim_handler

Summary

The require check in claim_fee only checks if the unclaimed fee is greater than zero and the and the amount of tokens in vault is greater than zero. This check does not consider checking whether the amount of tokens in the vault can settle the unclaimed fees.

Root Cause

On line 41 in the claim_fee.rs it doesn't check whether the balance in the vault will be sufficient to pay unclaimed fee

Internal pre-conditions

the unclaimed fee must be greater than zero but less than the token vault balance

External pre-conditions

no external pre-con

Attack Path

call claim_handler when unclaimed_fees is greater than 0 but less than token vault balance

Impact

This makes the require check ineffective because it is not able to catch these cases that its supposed to and this will lead to the program crashing

PoC

pub fn claim_handler(ctx: Context<ClaimFee>) -> Result<()> {
    let token_vault = &ctx.accounts.token_vault;
    let claim_fee_to_account = &ctx.accounts.claim_fee_to_account;
    let woopool = &mut ctx.accounts.woopool;

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

    let claim_amount = woopool.unclaimed_fee;
    woopool.unclaimed_fee = 0;

    transfer_from_vault_to_owner(
        woopool,
        token_vault,
        claim_fee_to_account,
        &ctx.accounts.token_program,
        claim_amount as u64,
    )?;

...
}

from the code above we can see that the require check only validates the unclaimed_fee and token_vault.amount against zero and forgets cases like the token_vault might not have enough balance to fulfill the transfer which can lead to the crashing of the program since unclaimed_fees are transferred from the vault as we can see above.

Mitigation

pub fn claim_handler(ctx: Context<ClaimFee>) -> Result<()> {
    let token_vault = &ctx.accounts.token_vault;
    let claim_fee_to_account = &ctx.accounts.claim_fee_to_account;
    let woopool = &mut ctx.accounts.woopool;

    require!(
        //@audit incorrect check!
-        woopool.unclaimed_fee > 0 && token_vault.amount as u128 > 0,
+        woopool.unclaimed_fee > 0 && token_vault.amount as u128 >= 0woopool.unclaimed_fee,
        ErrorCode::ProtocolFeeNotEnough
    );

    let claim_amount = woopool.unclaimed_fee;
    woopool.unclaimed_fee = 0;

    transfer_from_vault_to_owner(
        woopool,
        token_vault,
        claim_fee_to_account,
        &ctx.accounts.token_program,
        claim_amount as u64,
    )?;

...
}

[toprince]

Valid, but low impact, it will fail when transfer.