diff --git a/PR_DESCRIPTION_944.md b/PR_DESCRIPTION_944.md new file mode 100644 index 00000000..76fd0870 --- /dev/null +++ b/PR_DESCRIPTION_944.md @@ -0,0 +1,264 @@ +# PR: fix(contract) — harden custody accounting and transitions (issue #944) + +> **Suggested title:** `fix(contract): harden custody accounting and transitions` +> +> **Branch:** `fix/issue-944-custody-hardening` +> **Fork PR:** CodingBabe-1/Finchippay-Solution#3 · **Upstream PR:** FinChippay/Finchippay-Solution#964 + +--- + +## 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 +**FinChippay#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 + +- [x] Bug fix +- [ ] New feature +- [x] Documentation update +- [ ] Refactor / chore +- [x] Smart contract change + +--- + +## Related issue + +- Closes **FinChippay#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 + `+ 1`; on wrap-around they could mint duplicate IDs or bypass bounds checks. +- **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 + +### 1. Checked counter arithmetic (lib.rs) + +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. + +### 2. 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. + +### 3. Emergency-withdrawal counter build fix (lib.rs) + +The `main` merge into this branch left a conflict-resolution artifact in +`initiate_emergency_withdrawal`: + +```rust +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. + +### 4. Documentation (README.md, docs/architecture.md) + +- **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 #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: + +1. **The fork PR diff is large by construction.** The fork's `master` is behind + upstream `main`, so GitHub's comparison for CodingBabe-1/Finchippay-Solution#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. +2. **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 + +- [x] `git diff --check` — clean (no whitespace errors) +- [x] Root `package.json` parses as valid JSON +- [x] 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()`. +- [x] 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 + +- [x] My code follows the project style +- [x] I've updated docs if needed +- [ ] No console errors or warnings +- [x] I've rebased on latest `main` (branch merged with upstream `main` at + `3f353aa`) +- [x] 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) diff --git a/contracts/finchippay-contract/README.md b/contracts/finchippay-contract/README.md index 4b9e20b8..28373fdd 100644 --- a/contracts/finchippay-contract/README.md +++ b/contracts/finchippay-contract/README.md @@ -53,6 +53,8 @@ Recipients can call `claim_stream` at any time to drain accrued tokens. Payers c - Multi-sig proposals have a minimum of `MIN_MULTISIG_AMOUNT` and can include an `expiration_ledger` to auto-expire abandoned proposals. - Multi-sig signer lists are checked for duplicates at creation time. - Self-transfers (from == to) are rejected for tips, escrows, streams, and multi-sig. +- Custody transitions use an instance-scoped re-entry lock and verify exact contract-balance deltas on outbound transfers. +- Per-token locked-balance accounting is increased on deposits and decreased before every tracked payout or refund; `rescue_tokens` can only withdraw the unlocked balance. - Batch sends are limited to `MAX_BATCH_SIZE` (50) recipients and amounts are pre-validated for atomicity. - All operational entry points require the contract to be initialized via `initialize()`. - **Balance reconciliation**: the contract keeps a cached `LastContractBalance` @@ -124,13 +126,13 @@ bash ../../scripts/deploy-contract.sh | `(admin_transfer,)` | `new_admin: Address` | `transfer_admin` | | `(tip, from, to)` | `amount: i128` | `send_tip` | | `(receipt, from)` | `index: u32` | `mint_receipt` | -| `(escrow_create, id)` | `(from, to, amount, release_ledger)` | `create_escrow` | -| `(escrow_claim, id)` | `(to, amount)` | `claim_escrow` | -| `(escrow_cancel, id)` | `(from, amount)` | `cancel_escrow` | -| `(stream_open, id)` | `(payer, recipient, rate, deposit)` | `open_stream` | -| `(stream_claim, id)` | `(recipient, amount)` | `claim_stream` | -| `(stream_topup, id)` | `(payer, amount)` | `top_up_stream` | -| `(stream_close, id)` | `(payer, refund)` | `close_stream` | +| `(escrow_created, id)` | `(from, to, amount, release_ledger)` | `create_escrow` | +| `(escrow_released, id)` | `(to, amount)` | `claim_escrow` | +| `(escrow_cancelled,)` | `(id, from, amount)` | `cancel_escrow` | +| `(stream_opened, id)` | `(payer, recipient, rate, deposit)` | `open_stream` | +| `(stream_claimed, id)` | `(recipient, amount)` | `claim_stream` | +| `(stream_topped_up,)` | `(id, payer, added, new_deposit)` | `top_up_stream` | +| `(stream_closed, id)` | `(payer, refund)` | `close_stream` | | `(multisig_create, id)` | `(proposer, recipient, amount, threshold)` | `create_multisig` | | `(multisig_approve, id)` | `(signer, current_approvals, threshold)` | `approve_multisig` | | `(multisig_executed, id)` | `(recipient, amount)` | `approve_multisig` (auto) | diff --git a/contracts/finchippay-contract/src/lib.rs b/contracts/finchippay-contract/src/lib.rs index b5b5c4f0..fbee1e43 100644 --- a/contracts/finchippay-contract/src/lib.rs +++ b/contracts/finchippay-contract/src/lib.rs @@ -1649,9 +1649,11 @@ impl FinchippayContract { .persistent() .get(&DataKey::Version) .unwrap_or(CONTRACT_VERSION); + let next_ver = current_ver.checked_add(1).expect("version overflow"); + env.deployer().update_current_contract_wasm(new_wasm_hash.clone()); env.storage() .persistent() - .set(&DataKey::Version, &(current_ver + 1)); + .set(&DataKey::Version, &next_ver); bump(&env, &DataKey::Version); env.storage() .persistent() @@ -1934,13 +1936,14 @@ impl FinchippayContract { status: EmergencyWithdrawalStatus::Pending, }; + let next_count = id.checked_add(1).expect("withdrawal count overflow"); env.storage() .persistent() .set(&DataKey::EmergencyWithdrawal(id), &withdrawal); bump_to_floor(&env, &DataKey::EmergencyWithdrawal(id)); env.storage() .persistent() - .set(&DataKey::EmergencyWithdrawalCount, &(id + 1)); + .set(&DataKey::EmergencyWithdrawalCount, &next_count); bump(&env, &DataKey::EmergencyWithdrawalCount); env.events().publish( @@ -2283,9 +2286,10 @@ impl FinchippayContract { .set(&DataKey::ReceiptRecord(from.clone(), count), &receipt); bump_to_floor(&env, &DataKey::ReceiptRecord(from.clone(), count)); + let next_count = count.checked_add(1).expect("receipt count overflow"); env.storage() .persistent() - .set(&DataKey::ReceiptCount(from.clone()), &(count + 1)); + .set(&DataKey::ReceiptCount(from.clone()), &next_count); bump(&env, &DataKey::ReceiptCount(from.clone())); // Increment global receipt count and store index mapping diff --git a/docs/architecture.md b/docs/architecture.md index 472a6b17..37cbe298 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -57,6 +57,7 @@ Key design decisions: - **Emergency pause**: the admin *or* a designated pauser can call `pause()`/`unpause()` to freeze all value-transferring operations (circuit breaker). Read-only queries remain accessible during pause. - **Upgradability**: admin can call `upgrade(new_wasm_hash)` to deploy security patches without state migration. Version counter is incremented on each upgrade. - **Bounded inputs**: escrow timelocks, stream deposits/rates, and multi-sig amounts are capped to prevent griefing, overflow, and permanent fund lock-up. +- **Custody safety**: value transitions are serialized by an instance lock, effects are committed before token calls, outbound deltas are verified exactly, and per-token locked balances prevent rescue operations from touching active funds. #### Roles @@ -92,14 +93,14 @@ carries the remaining fields needed to reconstruct state. | `upgraded` | `(upgraded,)` | `(new_version, new_wasm_hash)` | `upgrade` | | `tip` | `(tip, from, to)` | `amount` | `send_tip` | | `receipt` | `(receipt, from)` | `index` | `mint_receipt` | -| `escrow_create` | `(escrow_create, id)` | `(from, to, amount, release_ledger)` | `create_escrow` | -| `escrow_claim_partial` | `(escrow_claim_partial, id)` | `(to, claim_amount, remaining)` | `claim_escrow_partial` | -| `escrow_claim` | `(escrow_claim, id)` | `(to, amount)` | `claim_escrow` | +| `escrow_created` | `(escrow_created, id)` | `(from, to, amount, release_ledger)` | `create_escrow` | +| `escrow_partial_released` | `(escrow_partial_released, id)` | `(to, claim_amount, remaining)` | `claim_escrow_partial` | +| `escrow_released` | `(escrow_released, id)` | `(to, amount)` | `claim_escrow` | | `escrow_cancelled` | `(escrow_cancelled,)` | `(id, from, amount)` | `cancel_escrow` | -| `stream_open` | `(stream_open, id)` | `(payer, recipient, rate_per_ledger, deposit)` | `open_stream` | -| `stream_claim` | `(stream_claim, id)` | `(recipient, claimable)` | `claim_stream` | +| `stream_opened` | `(stream_opened, id)` | `(payer, recipient, rate_per_ledger, deposit)` | `open_stream` | +| `stream_claimed` | `(stream_claimed, id)` | `(recipient, claimable)` | `claim_stream` | | `stream_topped_up` | `(stream_topped_up,)` | `(id, payer, added, new_deposit)` | `top_up_stream` | -| `stream_close` | `(stream_close, id)` | `(payer, refund)` | `close_stream` | +| `stream_closed` | `(stream_closed, id)` | `(payer, refund)` | `close_stream` | | `stream_reject` | `(stream_reject, id)` | `(recipient, refund)` | `reject_stream` | | `stream_transfer` | `(stream_transfer, id)` | `(current_recipient, new_recipient)` | `transfer_stream` | | `multisig_create` | `(multisig_create, id)` | `(proposer, recipient, amount, threshold)` | `create_multisig` |