Skip to content

fee_collector::set_treasury emits no event and the treasury address isn't actually enforced by withdraw #27

Description

@abayomicornelius

Problem

fee_collector::withdraw (fee_collector/src/lib.rs lines 130-160) never checks the contract's actual token balance before attempting the transfer:

let token_client = token::Client::new(&env, &token);
token_client.transfer(&env.current_contract_address(), &recipient, &amount);

If amount exceeds the contract's real balance of token, this call traps inside the token contract's own implementation (a generic "insufficient balance" failure originating from the token, not from fee_collector), rather than returning a clean, fee_collector-specific error.

Why it matters

fee_collector already exposes get_balance (lines 179-183) specifically to answer "how much of this token do we actually hold" — but withdraw doesn't use it to pre-validate before attempting the transfer. The practical effect is a worse error-handling experience for admin tooling: instead of a typed FeeCollectorError variant the caller's code can match on (as it can for every other failure mode in this contract), a failed over-withdrawal surfaces as an opaque token-contract-level trap, which is harder to handle gracefully in client code and harder to distinguish from other kinds of token-transfer failures (e.g. a frozen/paused underlying asset, if the token happens to support that).

Proposed fix

Add a balance pre-check in withdraw:

let current_balance = Self::get_balance(env.clone(), token.clone());
if amount > current_balance {
    return Err(FeeCollectorError::InsufficientBalance);
}

requiring a new FeeCollectorError::InsufficientBalance variant (next discriminant: 6).

Edge cases

  • This is purely a UX/error-clarity improvement — it doesn't change what can be withdrawn, only how a doomed-to-fail withdrawal attempt is reported. Both before and after this fix, the actual funds-safety property (can't withdraw more than the contract holds) is already correctly enforced by the underlying token contract; this fix just makes the failure legible.
  • Should confirm get_balance's cross-contract call (itself a token::Client::new(...).balance(...) call) doesn't introduce a meaningfully different gas/resource cost profile for withdraw given it's now doing two cross-contract calls (balance check + transfer) instead of one — likely negligible, but worth a quick before/after resource-cost comparison in the PR.

Testing strategy

Add test_withdraw_rejects_amount_exceeding_balance asserting Err(FeeCollectorError::InsufficientBalance) rather than a panic/trap, using a mock token client seeded with a known, smaller balance.

Metadata

Metadata

Assignees

No one assigned

    Labels

    GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26bugSomething isn't workingcontractsSmart contract logicvery hardVery difficult / senior-level bounty issue

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions