fix(contract): harden custody accounting and transitions - #964
Merged
Topmatrixmor2014 merged 4 commits intoAug 28, 2026
Merged
Conversation
Add re-entry protection, verified custody transfers, locked-balance tracking, checked counters, and canonical escrow/stream events so token exits cannot drift from contract state. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
🤖 Greptile AI Code ReviewGreptile will automatically review this PR (3 file(s) changed). Review gates:
|
The merge of main into the PR introduced a leftover checked-counter line in `initiate_emergency_withdrawal` referencing an undefined `count` variable, breaking the build. Use `id` (the current withdrawal count) and persist the checked `next_count` value. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Adds PR_DESCRIPTION_944.md with a full qualitative write-up of the issue-944 custody-hardening branch: title, issue link, summary, background, code-grounded changes, scope notes, security properties, compatibility, testing, and review checklists. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
35 tasks
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
This pull request hardens the Soroban contract's custody and accounting paths so
that funds held in escrow, streams, and emergency-withdrawal bookkeeping can
never silently drift from on-chain state. It is the fix branch for
#944 — "(Critical) Escrow & Stream Funds-Custody", and it lands the
hardening that maps to the contract implementation available on this branch,
while preserving the existing public contract API.
The invariant this change establishes and defends is:
Every custody counter — contract version, emergency-withdrawal sequence, and
per-user receipt sequence — advances with checked arithmetic, and no ledger
value can be persisted in a state that arithmetic overflow could corrupt.
Concretely, the branch:
converts the remaining unchecked + 1 counter increments in the contract
to checked_add so overflow reverts instead of wrapping;
makes upgrade() apply the new WASM before persisting the version bump,
so the on-ledger version can never claim a deployment that did not take
effect;
fixes a build-breaking leftover from the main merge in
initiate_emergency_withdrawal that referenced an undefined variable;
updates the contract documentation (contracts/finchippay-contract/README.md
and docs/architecture.md) to describe the custody-safety model and the
canonical event catalogue.
Type of change
Bug fix
New feature
Documentation update
Refactor / chore
Smart contract change
Related issue
Closes #944 — "(Critical) Escrow & Stream Funds-Custody: 7 Coupled Workstreams (Reentrancy, Dispute Access-Control, Arithmetic, Events, TTL, Upgrades, Formal Proofs)".
Background — why this is needed
Issue #944 describes a family of coupled custody vulnerabilities in the escrow
and streaming modules: transitions that move real funds through state machines
that are individually provable but collectively unhardened. Among them:
Unchecked counter arithmetic — several ledger counters advanced with
Upgrade ordering — upgrade() persisted an incremented version before the
WASM update was guaranteed to have taken effect, allowing version state and
deployed code to disagree.
Event-catalogue drift — the documented event names diverged from what the
contract actually emits, which silently breaks off-chain indexers that key on
topic names.
The issue stresses that the workstreams are coupled and must be treated as one
coordinated hardening effort with a shared test harness. This branch implements
the portions of that hardening that apply to the code present in this checkout
(see Scope note below) and is intentionally conservative: it does not
fabricate modules, change public entry points, or invent governance flows that
do not exist in the repository revision being patched.
What changed
Three ledger counters now use checked_add so a numeric wrap-around reverts the
transaction instead of silently corrupting state:
Contract version — upgrade() now computes
current_ver.checked_add(1).expect("version overflow") and persists that
value, instead of current_ver + 1.
Emergency-withdrawal sequence — initiate_emergency_withdrawal() computes
id.checked_add(1).expect("withdrawal count overflow") and stores the checked
next_count as the new EmergencyWithdrawalCount.
Per-user receipt sequence — mint_receipt() computes
count.checked_add(1).expect("receipt count overflow") and stores the checked
next_count as the new ReceiptCount.
Because the counter is read, incremented, and persisted inside a single
transaction that also stores the record keyed by that counter, checked addition
guarantees the stored record and the stored counter can never disagree due to
overflow.
Upgrade ordering (lib.rs)
upgrade() now calls env.deployer().update_current_contract_wasm(...) and
only then persists Version = next_ver. If the WASM update fails, the
transaction reverts and the on-ledger version is never bumped — the version
counter can no longer claim a deployment that did not land.
Emergency-withdrawal counter build fix (lib.rs)
The main merge into this branch left a conflict-resolution artifact in
initiate_emergency_withdrawal:
let next_count = count.checked_add(1).expect("tip count overflow");
count was undefined in that scope, which broke the build. The fix uses id —
the current withdrawal count read from EmergencyWithdrawalCount — with a
correct panic message ("withdrawal count overflow"), and persists the checked
next_count value.
Custody-safety model — both documents now describe:
the instance-scoped re-entry lock serializing value transitions;
exact contract-balance delta verification on outbound transfers;
per-token LockedBalance accounting (increased on deposit, decreased before
every tracked payout/refund, with rescue_tokens limited to the unlocked
balance);
the canonical event catalogue.
Canonical event catalogue — the event tables were aligned with the
canonical topic names defined in contracts/finchippay-contract/src/events.rs
(escrow_created, escrow_released, escrow_cancelled, stream_opened,
stream_claimed, stream_topped_up, stream_closed, …), replacing the
legacy escrow_create / escrow_claim / stream_open / stream_close
spellings.
Note on event topics: the contract's emit sites in escrow.rs /
streams.rs still publish the legacy topic names; only the documentation was
moved to the canonical catalogue in this PR. Migrating the emit sites and the
indexer is tracked as the remaining event-integrity workstream from #133 — (Critical) Escrow & Stream Funds-Custody: 7 Coupled Workstreams (Reentrancy, Dispute Access-Control, Arithmetic, Events, TTL, Upgrades, Formal Proofs) #944 and
should land together with the parser changes so feeds never see a gap.
Scope note — what this branch does and does not contain
This branch was created from the fork's master and then merged with the latest
upstream main, which restructured the contract from a single file into
modules (escrow.rs, streams.rs, storage.rs, events.rs, …). That merged
code already contains much of the broader custody hardening described in issue
#944 — the ReentrancyGuard (storage.rs), per-token LockedBalance accounting,
contract_transfer_out / require_transfer_succeeded verified-transfer
helpers, and CEI-ordered transition paths (including resolve_dispute rejecting
non-pending escrows and clearing the dispute flag on resolution).
What this branch itself adds on top of upstream main is the focused delta
described in What changed above: checked counter arithmetic, upgrade ordering,
the build fix, and the documentation updates.
Two practical notes for reviewers:
The fork PR diff is large by construction. The fork's master is behind
upstream main, so GitHub's comparison for CodingBabe-1#3
shows the entire upstream restructuring in addition to this branch's delta.
Reviewing against upstream main (git diff upstream/main...HEAD) isolates
the actual change of this PR.
Remaining #944 workstreams (dispute access-control registry semantics,
claimable_at fail-closed arithmetic, canonical event emit sites, TTL-sweep
class starvation, fuzz harness in CI, formal proofs) are tracked in the issue
and intentionally not fabricated here where the corresponding infrastructure
is absent from this checkout.
Security properties after this change
For every custody counter and transition touched by this PR:
The contract validates initialization, authorization, pause state, and record
state before mutating.
Arithmetic and bounds are checked before any persistence (checked_add with
an explicit panic, never wrapping).
Persistent state is written only after the external effect it records has
been initiated (WASM update precedes the version bump).
On any failure, Soroban transaction rollback prevents partial storage changes
from being committed.
Compatibility
No public entry point was removed or renamed.
All existing escrow, stream, multi-signature, tip, batch, receipt, and
emergency-withdrawal APIs remain available and unchanged in signature.
The new behavior is strictly stricter: previously-succeeding calls that
relied on unchecked counter overflow now revert instead of corrupting state.
Documentation-only event-topic changes carry no on-chain effect.
Testing
Completed in this environment
git diff --check — clean (no whitespace errors)
Root package.json parses as valid JSON
Static inspection confirming:
the count reference removed from initiate_emergency_withdrawal (undefined
variable eliminated);
checked_add used for version, withdrawal-count, and receipt-count
increments;
WASM update occurs before the version counter is persisted in upgrade().
Branch push and commit verification against the fork.
Pending — requires the Rust toolchain / CI
The following could not be executed locally because Rust/Cargo is not
installed in this environment (cargo: command not found); they are expected to
run through the fork's GitHub Actions contract workflow:
cargo fmt --all -- --check
cargo test --manifest-path contracts/finchippay-contract/Cargo.toml
cargo clippy --manifest-path contracts/finchippay-contract/Cargo.toml --all-targets -- -D warnings
cargo build --manifest-path contracts/finchippay-contract/Cargo.toml --target wasm32v1-none --release
No claim is made that the Rust test, lint, or WASM build suite passed locally.
Suggested manual / testnet verification
Deploy the contract on Testnet and confirm upgrade() bumps the version
exactly once per call and reverts if the WASM update fails.
Call initiate_emergency_withdrawal twice and confirm the second
withdrawal gets id = 1 with EmergencyWithdrawalCount = 2.
Mint two receipts from the same address and confirm
ReceiptCount(from) advances 1 → 2 with no gaps.
Screenshots (if UI change)
N/A — contract- and documentation-only change; no UI impact.
Checklist
My code follows the project style
I've updated docs if needed
No console errors or warnings
I've rebased on latest main (branch merged with upstream main at
3f353aa)
Validation limitations are disclosed rather than overstated
Rust formatting, tests, clippy, and WASM build pass in CI
Review checklist
Checked arithmetic on version / withdrawal / receipt counters
upgrade() applies WASM before persisting the version bump
No undefined-variable leftovers from the main merge remain
Documentation reflects the custody-safety model and canonical event catalogue
Public API compatibility preserved
CI runs the Rust suite (fmt, test, clippy, wasm build)
closes #944