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.
Problem
fee_collector::withdraw(fee_collector/src/lib.rslines 130-160) never checks the contract's actual token balance before attempting the transfer:If
amountexceeds the contract's real balance oftoken, this call traps inside the token contract's own implementation (a generic "insufficient balance" failure originating from the token, not fromfee_collector), rather than returning a clean,fee_collector-specific error.Why it matters
fee_collectoralready exposesget_balance(lines 179-183) specifically to answer "how much of this token do we actually hold" — butwithdrawdoesn'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 typedFeeCollectorErrorvariant 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:requiring a new
FeeCollectorError::InsufficientBalancevariant (next discriminant: 6).Edge cases
get_balance's cross-contract call (itself atoken::Client::new(...).balance(...)call) doesn't introduce a meaningfully different gas/resource cost profile forwithdrawgiven 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_balanceassertingErr(FeeCollectorError::InsufficientBalance)rather than a panic/trap, using a mock token client seeded with a known, smaller balance.