Background / Context
escrow::fund (contracts/escrow/src/lib.rs:54-88) accepts a deadline: u64 parameter with zero validation — there is no check that deadline > env.ledger().timestamp() (i.e., that the deadline is actually in the future relative to funding time) and no check against created_at at all. refund's permissionless-after-deadline path (contracts/escrow/src/lib.rs:151-156) compares now < escrow.deadline to decide whether admin authorization is still required.
Problem Statement
Because deadline is caller-supplied at fund time with no floor, a sponsor can fund an escrow with deadline = 0 (or any timestamp already in the past), making it refundable by anyone immediately — which for the sponsor's own funds is harmless (it's their money; they can already call refund themselves at any time regardless of deadline status? — verify: actually refund only skips the admin-auth requirement once now >= deadline; the sponsor still isn't special-cased at all in refund, so a sponsor with a future deadline currently has no way to self-refund early without going through the admin — is that intentional? This needs to be resolved as part of this audit). Separately and more importantly: consider whether a malicious actor (not the sponsor) gains anything from a degenerate deadline — since fund requires sponsor.require_auth(), only the sponsor (or someone with the sponsor's authorization) can set the deadline for their own escrow, limiting griefing-of-others potential, but this should be explicitly verified rather than assumed, especially in combination with the separate fund-front-running/id-squatting issue (an attacker squatting an issue_id could set deadline=0, making their own worthless deposit immediately refundable by anyone — is that actually a mitigation or irrelevant to that attack? Analyze the interaction).
Requirements
- Determine definitively whether a sponsor can currently self-refund before
deadline without admin involvement (read refund carefully: the only auth-bypass condition is now >= deadline; before that, require_admin(&env)?.require_auth() is called — meaning the admin's signature is required, not the sponsor's, even for the sponsor's own funds). If this is unintended, this is itself a distinct usability/trust bug worth fixing (sponsors should very plausibly be able to cancel their own not-yet-released funding without needing the oracle's cooperation) — propose and implement a sponsor.require_auth()-gated early-refund path as an alternative to the current admin-only early path.
- Add explicit validation to
fund: reject deadline <= env.ledger().timestamp() (or a documented minimum lead time) with a new or existing Error variant, to prevent degenerate zero/past deadlines from being createable at all, closing off the ambiguity rather than relying on downstream logic happening to be safe.
- Fully analyze and document the interaction with the id-squatting/front-running issue tracked separately — does a
deadline=0 squatted escrow actually help or hinder recovery from that attack, and should the fix for one issue inform the fix for the other?
- Add tests covering:
fund rejects non-future deadlines, sponsor-initiated early refund (if implemented) works correctly and only for the actual sponsor, and admin early-refund continues to work as before.
Acceptance Criteria
Technical Notes / Hints
Error::NotExpired already exists (error.rs:16) but is currently unused anywhere in lib.rs — check whether it was intended for exactly this kind of validation and was simply never wired up.- Note carefully: the sponsor field is stored in the
Escrow struct — any new self-refund path must check escrow.sponsor == caller semantics via require_auth on the stored sponsor address, not a caller-supplied one, to avoid introducing a new spoofing vector.
Difficulty Justification
This requires carefully reading and correctly interpreting existing, subtly-scoped authorization logic (the current early-refund path's admin-only, not-sponsor requirement is easy to misread as already covering the sponsor), reasoning about interaction effects with a separate tracked vulnerability rather than fixing this in isolation, and correctly implementing a new self-service authorization path without introducing a spoofing bug in the process — plus noticing and explaining the suspiciously unused Error::NotExpired variant.
Background / Context
escrow::fund(contracts/escrow/src/lib.rs:54-88) accepts adeadline: u64parameter with zero validation — there is no check thatdeadline > env.ledger().timestamp()(i.e., that the deadline is actually in the future relative to funding time) and no check againstcreated_atat all.refund's permissionless-after-deadline path (contracts/escrow/src/lib.rs:151-156) comparesnow < escrow.deadlineto decide whether admin authorization is still required.Problem Statement
Because
deadlineis caller-supplied atfundtime with no floor, a sponsor can fund an escrow withdeadline = 0(or any timestamp already in the past), making it refundable by anyone immediately — which for the sponsor's own funds is harmless (it's their money; they can already callrefundthemselves at any time regardless of deadline status? — verify: actuallyrefundonly skips the admin-auth requirement oncenow >= deadline; the sponsor still isn't special-cased at all inrefund, so a sponsor with a future deadline currently has no way to self-refund early without going through the admin — is that intentional? This needs to be resolved as part of this audit). Separately and more importantly: consider whether a malicious actor (not the sponsor) gains anything from a degenerate deadline — sincefundrequiressponsor.require_auth(), only the sponsor (or someone with the sponsor's authorization) can set the deadline for their own escrow, limiting griefing-of-others potential, but this should be explicitly verified rather than assumed, especially in combination with the separatefund-front-running/id-squatting issue (an attacker squatting anissue_idcould setdeadline=0, making their own worthless deposit immediately refundable by anyone — is that actually a mitigation or irrelevant to that attack? Analyze the interaction).Requirements
deadlinewithout admin involvement (readrefundcarefully: the only auth-bypass condition isnow >= deadline; before that,require_admin(&env)?.require_auth()is called — meaning the admin's signature is required, not the sponsor's, even for the sponsor's own funds). If this is unintended, this is itself a distinct usability/trust bug worth fixing (sponsors should very plausibly be able to cancel their own not-yet-released funding without needing the oracle's cooperation) — propose and implement asponsor.require_auth()-gated early-refund path as an alternative to the current admin-only early path.fund: rejectdeadline <= env.ledger().timestamp()(or a documented minimum lead time) with a new or existingErrorvariant, to prevent degenerate zero/past deadlines from being createable at all, closing off the ambiguity rather than relying on downstream logic happening to be safe.deadline=0squatted escrow actually help or hinder recovery from that attack, and should the fix for one issue inform the fix for the other?fundrejects non-future deadlines, sponsor-initiated early refund (if implemented) works correctly and only for the actual sponsor, and admin early-refund continues to work as before.Acceptance Criteria
fundvalidatesdeadlineis meaningfully in the futurerequire_authscoping to the actualescrow.sponsor, not just any caller)cargo test --workspacepassesTechnical Notes / Hints
Error::NotExpiredalready exists (error.rs:16) but is currently unused anywhere inlib.rs— check whether it was intended for exactly this kind of validation and was simply never wired up.Escrowstruct — any new self-refund path must checkescrow.sponsor == callersemantics viarequire_authon the stored sponsor address, not a caller-supplied one, to avoid introducing a new spoofing vector.Difficulty Justification
This requires carefully reading and correctly interpreting existing, subtly-scoped authorization logic (the current early-refund path's admin-only, not-sponsor requirement is easy to misread as already covering the sponsor), reasoning about interaction effects with a separate tracked vulnerability rather than fixing this in isolation, and correctly implementing a new self-service authorization path without introducing a spoofing bug in the process — plus noticing and explaining the suspiciously unused
Error::NotExpiredvariant.