Skip to content

fix(escrow): centralize the reclaim boundary and return only held funds - #262

Merged
Cjay-Cyber-2 merged 5 commits into
ASTROIDX556:mainfrom
DevRushd:fix/escrow-reclaim-mechanics
Oct 7, 2026
Merged

Cjay-Cyber-2 merged 5 commits into
ASTROIDX556:mainfrom
DevRushd:fix/escrow-reclaim-mechanics

Conversation

@DevRushd

Copy link
Copy Markdown
Contributor

Closes #225

What this does

Refactors escrow expiration handling so the reclaim boundary exists once, and reworks fund reclaim so every settlement path returns only what the escrow still holds.

1. One reclaim boundary, computed safely

  • New private helper grace_end(escrow) = deadline.saturating_add(grace_period) replaces seven inline deadline + grace_period computations (claim x2, release, expire, refund, refund_timelock, is_refundable) plus reclaim's ad-hoc checked_add(deadline as i128, grace_period as i128)? as u64 and the first half of closes_at.
  • The raw + panics on an overflowing u64 sum; the i128 roundtrip is worse — it computes a valid i128 sum and then truncates it back to u64, which can wrap the boundary to a past instant and open a reclaim while the arbiter can still release.
  • New view refund_opens_at(id) exposes the boundary to clients (pairs with the existing refund_window_closes_at, which now measures from it).
  • create_with_refund_window (and therefore create) and initialize_timelock reject a grace_period that would saturate the boundary with Error::InvalidInput, before any state is written, so no escrow can be created whose reclaim path never opens (permanent lockup).

2. refund was missing its party check

refund called caller.require_auth() and then verified nothing about caller — any address could force-settle someone else's escrow after the deadline. It is now sender-only, matching refund_timelock and reclaim (funds always return to the depositor).

refund and reclaim also accept Created alongside Funded | Expired, so the reclaim paths agree with the is_refundable view that already advertised Created | Funded | Expired. Unfunded timelock escrows can now be settled instead of being stuck between InvalidState and a trapping transfer.

3. Reclaims return only the remainder (disburse_remainder)

Previously reclaim and cancel used transfer_all, sending each asset's original amount even when the recipient had already withdrawn part of a vested escrow. With 6_000 of a 10_000 escrow left in custody that transfer fails; with a second escrow's funds sitting in the same contract it would silently drain them.

All six paths that empty an escrow — refund, refund_timelock, reclaim, cancel, release, override_release — now go through one helper that pro-rates amount * (funded_amount - released_amount) / funded_amount per asset and emits the shared ("transfer", "executed") event for every amount actually moved (refund/refund_timelock previously emitted none).

Supporting fixes required to make the invariant true:

  • release_milestone mirrors set.released_amount onto escrow.released_amount (previously only stored on the final milestone), so a partially released milestone escrow can no longer look untouched.
  • override_release records released_amount = funded_amount instead of leaving it stale on a Released record.
  • claim on an unfunded Created escrow returns InvalidState instead of attempting a transfer the contract cannot cover; cancel on the same record now transitions cleanly with no transfer.

Doc corrections

  • close's summary sat above cancel, and validate_assets' summary sat above closes_at; both are restored to their functions.
  • release's doc claimed partial releases are supported while the implementation always pays the whole remainder; the doc now describes the real release_amount semantics (upper bound, remainder paid) — behaviour is unchanged.
  • release's "before the deadline" corrected to "until the reclaim boundary is reached", and the module docs gained a "Reclaim rules" section.

Tests

contracts/escrow/src/test.rs gains 8 tests (46 → 54, all green):

test covers
refund_opens_at_reports_the_reclaim_boundary boundary one second early (reclaim/refund → GraceActive, expire → InvalidState, is_refundable false) and exactly at it (all three open, refund succeeds)
refund_rejected_for_non_sender recipient, arbiter and a stranger all get Unauthorized; sender still succeeds
reclaim_after_partial_withdrawal_returns_only_the_remainder recipient withdraws 4_000 of 10_000; reclaim returns 6_000, never 10_000
reclaim_after_partial_milestone_release_returns_only_the_remainder first milestone paid (40%); reclaim returns the outstanding 6_000 and released_amount is mirrored
cancel_after_partial_withdrawal_returns_only_the_remainder mid-flight cancel returns 7_000, recipient keeps 3_000
release_after_partial_withdrawal_pays_only_the_remainder arbiter settlement pays the outstanding 7_000, not the original 10_000
unfunded_timelock_escrow_has_a_clean_reclaim_path is_refundable/refund agree on Created; claim refuses instead of trapping
absurd_grace_period_is_refused_at_creation create and initialize_timelock reject u64::MAX grace with InvalidInput, no tokens pulled, id counter untouched

Validation (CI commands)

cargo test --package astroid-escrow   54 passed, 0 failed (46 pre-existing + 8 new)
cargo test --workspace                all suites green
cargo fmt --check                     clean
cargo clippy --workspace -- -D warnings   clean

No emojis; no collaborator/contributor additions.

Expiration handling: every entrypoint that settled `deadline +
grace_period` inline now reads one saturating `grace_end` helper,
surfaced to clients as the new `refund_opens_at` view. The raw `+`
could panic on an overflowing sum, while `reclaim`'s
`checked_add(deadline as i128, grace_period as i128)? as u64` could
truncate a wrapped sum back to a past instant and open a reclaim while
the arbiter could still release. Creation now rejects a grace period
that would saturate the boundary with `Error::InvalidInput`, so no
escrow can be created with a reclaim path that never opens.

Reclaim mechanics: `refund` had no party check at all — any address
could settle it. It is now sender-only, like `refund_timelock` and
`reclaim`, and all three accept `Created` alongside `Funded`/`Expired`
so the reclaim paths agree with the `is_refundable` view.

Every path that empties an escrow (`refund`, `refund_timelock`,
`reclaim`, `cancel`, `release`, `override_release`) now moves only the
un-released remainder through one `disburse_remainder` helper, pro-rated
per asset, instead of `transfer_all`. Previously a reclaim after a
partial recipient withdrawal asked the contract for the full original
amount, which would fail — or, with another escrow's funds in custody,
drain it. `release_milestone` mirrors its bookkeeping onto
`escrow.released_amount` so partially released milestone escrows are
covered too, and `override_release` no longer leaves that field stale.

Also: an unfunded `Created` escrow can no longer be driven into a token
transfer it cannot cover by `claim` or `cancel`, and two misplaced doc
comments (`close`'s summary above `cancel`, `validate_assets`' summary
above `closes_at`) are back where they belong.

Tests: 8 added (46 -> 54) covering the boundary second-by-second, the
missing `refund` sender gate, remainder-only reclaims after a partial
withdrawal and after a partial milestone release, cancel/release after
a partial withdrawal, the unfunded reclaim path, and grace-period
overflow rejection at creation.
@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

Merge Conflict — Action Needed

This pull request has merge conflicts with the base branch (main).

What to do: update your branch by merging or rebasing against main, resolve any conflicts locally, and push the result.

@drips-wave

drips-wave Bot commented Sep 25, 2026

Copy link
Copy Markdown

@DevRushd Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

CI Checks Failed — Action Needed

The continuous integration checks on this pull request are failing. I can't merge until all required checks pass.

Failing checks:

  • test: failure

What to do: review the failing checks in the "Checks" tab above, fix the issues in your code, and push the changes.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

Merge Conflict — Action Needed

This pull request has merge conflicts with the base branch (main).

What to do: update your branch by merging or rebasing against main, resolve any conflicts locally, and push the result.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

CI Checks Failed — Action Needed

The continuous integration checks on this pull request are failing. I can't merge until all required checks pass.

Failing checks:

  • test: failure

What to do: review the failing checks in the "Checks" tab above, fix the issues in your code, and push the changes.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

Merge Conflict — Action Needed

This pull request has merge conflicts with the base branch (main).

What to do: update your branch by merging or rebasing against main, resolve any conflicts locally, and push the result.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

CI Checks Failed — Action Needed

The continuous integration checks on this pull request are failing. I can't merge until all required checks pass.

Failing checks:

  • test: failure

What to do: review the failing checks in the "Checks" tab above, fix the issues in your code, and push the changes.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

Merge Conflict — Action Needed

This pull request has merge conflicts with the base branch (main).

What to do: update your branch by merging or rebasing against main, resolve any conflicts locally, and push the result.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

CI Checks Failed — Action Needed

The continuous integration checks on this pull request are failing. I can't merge until all required checks pass.

Failing checks:

  • test: failure

What to do: review the failing checks in the "Checks" tab above, fix the issues in your code, and push the changes.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

Merge Conflict — Action Needed

This pull request has merge conflicts with the base branch (main).

What to do: update your branch by merging or rebasing against main, resolve any conflicts locally, and push the result.

1 similar comment
@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

Merge Conflict — Action Needed

This pull request has merge conflicts with the base branch (main).

What to do: update your branch by merging or rebasing against main, resolve any conflicts locally, and push the result.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

CI Checks Failed — Action Needed

The continuous integration checks on this pull request are failing. I can't merge until all required checks pass.

Failing checks:

  • test: failure

What to do: review the failing checks in the "Checks" tab above, fix the issues in your code, and push the changes.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

Merge Conflict — Action Needed

This pull request has merge conflicts with the base branch (main).

What to do: update your branch by merging or rebasing against main, resolve any conflicts locally, and push the result.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

CI Checks Failed — Action Needed

The continuous integration checks on this pull request are failing. I can't merge until all required checks pass.

Failing checks:

  • test: failure

What to do: review the failing checks in the "Checks" tab above, fix the issues in your code, and push the changes.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

Merge Conflict — Action Needed

This pull request has merge conflicts with the base branch (main).

What to do: update your branch by merging or rebasing against main, resolve any conflicts locally, and push the result.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

CI Checks Failed — Action Needed

The continuous integration checks on this pull request are failing. I can't merge until all required checks pass.

Failing checks:

  • test: failure

What to do: review the failing checks in the "Checks" tab above, fix the issues in your code, and push the changes.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

Merge Conflict — Action Needed

This pull request has merge conflicts with the base branch (main).

What to do: update your branch by merging or rebasing against main, resolve any conflicts locally, and push the result.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

CI Checks Failed — Action Needed

The continuous integration checks on this pull request are failing. I can't merge until all required checks pass.

Failing checks:

  • test: failure

What to do: review the failing checks in the "Checks" tab above, fix the issues in your code, and push the changes.

@Cjay-Cyber-2

Copy link
Copy Markdown
Contributor

Merge Conflict — Action Needed

This pull request has merge conflicts with the base branch (main).

What to do: update your branch by merging or rebasing against main, resolve any conflicts locally, and push the result.

@Cjay-Cyber-2
Cjay-Cyber-2 merged commit a8024df into ASTROIDX556:main Oct 7, 2026
1 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor escrow contract expiration handling and fund reclaim mechanics

2 participants