Skip to content

fix(escrow): reject sender == recipient self-escrows in deposit() - #543

Open
Vyacheslav-Tomashevskiy wants to merge 1 commit into
Northgate-Systems:mainfrom
Vyacheslav-Tomashevskiy:fix/329-prevent-self-escrow
Open

fix(escrow): reject sender == recipient self-escrows in deposit()#543
Vyacheslav-Tomashevskiy wants to merge 1 commit into
Northgate-Systems:mainfrom
Vyacheslav-Tomashevskiy:fix/329-prevent-self-escrow

Conversation

@Vyacheslav-Tomashevskiy

Copy link
Copy Markdown
Contributor

Summary

deposit() had no explicit handling for sender == recipient (a self-escrow). This adds a guard that rejects it with a clear panic, decided in favor of "prevent" rather than "allow + document" per the issue's either/or framing: a self-escrow has no legitimate use in a remittance flow (the same party would be both who can be refunded on expiry and who release() would eventually pay out to), and it just ties up funds behind the still-undecided release() authorization mechanism for zero benefit.

What changed

  • contracts/escrow/src/lib.rs: added the sender == recipient check right after require_auth() in deposit(), before the amount/expiry validation.
  • contracts/escrow/src/test.rs: added test_deposit_rejects_self_escrow (failure path). The success path (sender != recipient) is already covered by every existing deposit() test, so per the issue's acceptance criteria there's no new success-path test needed - all 13 pre-existing tests pass unchanged.
  • contracts/escrow/README.md: short note on the new validation rule under the interface table.

Testing

  • cargo test: 14/14 pass (13 pre-existing + 1 new).
  • cargo clippy --all-targets -- -D warnings: clean, no warnings.
  • cargo fmt --check: shows the same pre-existing diffs already on a clean main checkout (verified before making any changes) - none touch code this PR modifies, left alone rather than reformatting unrelated lines.
  • cargo build --target wasm32-unknown-unknown --release: fails identically on this branch and on a clean main checkout (pre-existing testutils-feature issue, unrelated, already flagged to the maintainer in earlier PRs).

Checklist (from the issue)

  • Behavior implemented in contracts/escrow/src/lib.rs
  • New test covering the failure path (success path already covered)
  • cargo test passes locally
  • cargo clippy -- -D warnings and cargo fmt --check pass (fmt: no new diffs introduced)
  • Doc comment added explaining the decision
  • README.md updated

Closes #329

A sender escrowing funds to themselves had no explicit handling -
deposit() would happily lock funds where sender and recipient are the
same address. This has no legitimate use in a remittance flow (the
same party would be both who can be refunded on expiry and who
release() would eventually pay out to) and just ties up funds behind
the still-undecided release() authorization mechanism for zero
benefit, so it's now rejected with a clear panic instead of silently
allowed.

Added the check right after require_auth() in deposit(), before the
amount/expiry validation, so it's the first thing rejected on a bad
call.

Tests: added test_deposit_rejects_self_escrow (failure path). The
success path (sender != recipient) was already covered by every
existing deposit test, so no new success-path test was needed per the
issue's acceptance criteria - all 13 pre-existing tests still pass
unchanged. 14/14 total. cargo clippy --all-targets -- -D warnings is
clean. cargo fmt --check shows the same pre-existing diffs already on
main (verified against a clean main checkout before making any
changes) - none of them touch code this PR modifies.

README updated with a short note on the new validation rule.

Closes Northgate-Systems#329
@vercel

vercel Bot commented Sep 9, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the codex723's projects Team on Vercel.

A member of the Team first needs to authorize it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prevent sender == recipient self-escrows (or explicitly allow + document)

1 participant