fix(contract): close circuit-breaker pause coverage gaps and add completeness audit - #659
Open
iyanumajekodunmi756 wants to merge 1 commit into
Open
Conversation
…leteness audit
The emergency pause (pause/unpause/require_not_paused) was not applied to
every value-transferring entry point, so funds could keep moving while the
contract was "paused". Close every coverage gap, check the audit matrix into
docs, and lock the behavior in with an integration suite.
- Guard the previously-unguarded fund-moving paths: legacy rescue_tokens,
the rescue_tokens admin action, resolve_dispute, and all airdrop and
yield-escrow operations.
- Add docs/pause-completeness.md: the authoritative matrix mapping every
entry point to {mutating, value-transferring, pause-guarded}.
- Add tests/pause_completeness.rs: pauses the contract and asserts every
value-transferring entry point is blocked (with no funds moved), every
read-only view still works, and the governance escape hatches (pause/
unpause, admin multi-sig, raise_dispute, etc.) remain callable.
- No pause semantics changed; the pauser/legacy-admin distinction is intact.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
🤖 Greptile AI Code ReviewGreptile will automatically review this PR (6 file(s) changed). Review gates:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #623 — Circuit-Breaker (Pause) Completeness Audit Across All Entrypoints (Reward Tier: 🏆 S).
This is a completeness-and-proof security fix, not a new feature. The contract already had
pause/unpause/require_not_paused, but the guard was not uniformly applied across every value-transferring entry point — meaning an attacker could keep moving funds while the contract was "paused". This PR:require_not_pausedto every fund-moving path that was missing it.docs/pause-completeness.mdmaps every entry point to{mutating, value-transferring, pause-guarded}.tests/pause_completeness.rspauses the contract and asserts every value-transferring entry point is blocked (≥10 assertions), every read-only view still works, and no funds move while paused.No pause semantics were changed: the pauser/legacy-admin distinction is untouched, the guard still panics with the same
"Contract is paused"message (ContractError::ContractPaused), and only coverage gaps were closed.What was missing (the bug)
Value transfer is spread across many modules (
escrow.rs,streams.rs,multi_sig.rs,batch_send.rs,airdrop.rs,yield_escrow.rs, and the swap/DEX + rescue paths inlib.rs). Auditing everypub fnentry point surfaced nine fund-moving code paths that could move tokens while paused:rescue_tokens(legacy admin rescue)lib.rsrescue_tokensadmin action (multi-sig branch)lib.rsexecute_admin_actionresolve_dispute(arbitrator payout)escrow.rscreate_airdrop/claim_airdrop/cancel_airdropairdrop.rscreate_yield_escrow/claim_yield_escrow/cancel_yield_escrowyield_escrow.rsAll nine are now guarded with the existing
require_not_pausedhelper.What was already correct (and is now proven)
The following were already guarded and are now locked in by tests:
send_tip,create_escrow,claim_escrow,claim_escrow_partial,cancel_escrow,create_disputable_escrow,open_stream,claim_stream,top_up_stream,close_stream,reject_stream,transfer_stream,create_multisig,approve_multisig,timeout_multisig,cancel_multisig,batch_send,batch_send_multi,create_vesting,claim_vesting,revoke_vesting,swap_exact_tokens_for_tokens,swap_tokens_for_exact_tokens,initiate_emergency_withdrawal,approve_emergency_withdrawal,execute_emergency_withdrawal,cancel_emergency_withdrawal, andmint_receipt.Deliberately NOT pause-guarded (by design)
pause/unpauseand the admin multi-sigpropose_admin_action/approve_admin_actionmust stay callable while paused so the circuit breaker can always be lifted. The only value-transferring branch they dispatch —rescue_tokens— is individually guarded insideexecute_admin_action.raise_disputeonly flags state and moves no funds, so it deliberately remains callable while paused.get_escrow,get_stream,get_multisig,get_admin,get_claimable,get_vesting,get_emergency_withdrawal,get_contract_stats, tip/receipt getters, estimates, etc.) keep working while paused.Files changed
contracts/finchippay-contract/src/lib.rs— guard legacyrescue_tokensand therescue_tokensadmin-action branch; document the circuit-breaker invariant onrequire_not_paused.contracts/finchippay-contract/src/escrow.rs— guardresolve_dispute.contracts/finchippay-contract/src/airdrop.rs— guardcreate_airdrop/claim_airdrop/cancel_airdrop.contracts/finchippay-contract/src/yield_escrow.rs— guardcreate_yield_escrow/claim_yield_escrow/cancel_yield_escrow.docs/pause-completeness.md(new) — the authoritative audit matrix (all 92 entry points + internal module functions), semantics, and what the change fixed.contracts/finchippay-contract/tests/pause_completeness.rs(new) — the machine-checked integration suite that asserts the matrix holds.Test coverage (
tests/pause_completeness.rs)Every test deploys the contract, sets up real, fully-executable on-chain state before pausing (so the only thing that can block an operation is the pause guard), pauses, then asserts the operation is blocked and no funds moved:
test_pause_blocks_tips_and_receiptssend_tip,mint_receiptblocked; balances unchangedtest_pause_blocks_escrow_and_dispute_operationscreate_escrow,claim_escrow,claim_escrow_partial,cancel_escrow,create_disputable_escrow,resolve_disputeblocked; escrows stayPendingtest_pause_blocks_stream_operationsopen_stream,claim_stream,top_up_stream,close_stream,reject_stream,transfer_streamblocked; stream untouchedtest_pause_blocks_multisig_operationscreate_multisig,approve_multisig,timeout_multisig,cancel_multisigblocked; proposal staysPendingtest_pause_blocks_batch_and_vesting_operationsbatch_send,batch_send_multi,create_vesting,claim_vesting,revoke_vestingblockedtest_pause_blocks_swap_operationstest_pause_blocks_rescue_and_emergency_withdrawalrescue_tokens,initiate/approve/execute/cancel_emergency_withdrawalblockedtest_pause_blocks_rescue_tokens_admin_actionrescue_tokensadmin action is blocked even thoughpropose_admin_actionstays callabletest_views_work_while_pausedcheck_invariantsstill returns correct data while pausedtest_governance_escape_hatches_work_while_pausedpause/unpause(pauser), admin propose/approve,set_fee_collector,set_swap_fee,transfer_admin, arbitrator management,bump_all_ttls,raise_disputestill work while pausedtest_unpause_restores_value_transferunpause,send_tipsucceeds againThe
assert_blocked_by_pausehelper asserts the call errors out (the "equivalent panic" forContractError::ContractPaused) rather than merely checkingis_err()loosely — so a regression that removes the guard and lets the call through fails loudly.Acceptance criteria — verified ✅
{mutating, pauses}→docs/pause-completeness.md, fully green.test_views_work_while_pausedcoversget_escrow,get_stream,get_multisig,get_admin,get_admin_signers,get_claimable,get_vesting,get_emergency_withdrawal,get_contract_stats, tip/receipt getters, estimates, and more.cargo testpasses — 214 tests pass (203 baseline + 11 new).cargo build --target wasm32v1-none --releasepasses.cargo check --target wasm32v1-nonepasses (CI).cargo test --test integrationpasses (CI).How to verify
Notes
cargo fmt-clean at baseline, socargo fmt --checkfails on unrelated pre-existing code; the new test file isrustfmt-clean and the diff is minimal.pausesemantics changed;require_not_pausedremains the single source of truth for the"Contract is paused"panic.Closes #623