escrow: dead NotExpired/InsufficientBalance error variants suggest a missing defensive balance check in release()/refund()

Đang mở
  • Có gì không ổn?
#48 2 bình luận 0 reaction 1 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 1 ngày

@Devadakene đang làm issue này rồi.

Từ ngày 17/8/2026.

  • #70 của @Devadakene — đang mở

Đánh giá

Issue này chưa được đánh giá.

Mô tả

bug GrantFox OSS Maybe Rewarded Official Campaign | FWC26 testing Third Campaign very hard

Overview

contracts/escrow/src/error.rs defines two Error variants that are never constructed anywhere in contracts/escrow/src/lib.rs: NotExpired = 10 and InsufficientBalance = 11. Confirmed by cross-referencing every defined variant against every Error:: construction site in the file (grep -oE '^\s+[A-Za-z]+ = [0-9]+,' contracts/escrow/src/error.rs vs. grep -oE 'Error::[A-Za-z]+' contracts/escrow/src/lib.rs | sort -u): AlreadyInitialized, NotInitialized, EscrowNotFound, AlreadyFunded, AlreadyPaid, AlreadyRefunded, InvalidSplit, InvalidAmount, InvalidFee, InvalidDeadline, and (indirectly, via compute_split) the ones used there — but never NotExpired or InsufficientBalance.

docs/access-control-audit.md already documents that Unauthorized is dead code, and explains precisely why: Soroban's Address::require_auth() panics rather than returning a Result, so a failed auth check can never reach a point where the contract constructs Err(Error::Unauthorized). That explanation does not apply to NotExpired or InsufficientBalance — neither is auth-related, and both variant names describe exactly the kind of ordinary, Result-returnable validation failure this codebase handles correctly everywhere else (see AlreadyPaid/AlreadyRefunded/InvalidDeadline, all real, reachable, tested rejections). These two variants are neither explained by the audit doc nor accounted for anywhere else in this repository.

Their names are specific enough to suggest they're not arbitrary cruft:

  • NotExpired reads exactly like the error a deadline-gated function would return when called before its deadline — i.e., precisely the shape of check refund's early-path used to need before it was refactored to the current if now < escrow.deadline { admin required } else { permissionless } structure (contracts/escrow/src/lib.rs:161-166). It's plausible this variant is a relic from an earlier design where a non-admin caller attempting refund before expiry was rejected with a typed NotExpired error rather than relying on require_auth()'s panic — worth confirming against git history whether such a code path ever existed and was removed, or whether this was scaffolded in error.rs ahead of lib.rs and never wired up.
  • InsufficientBalance is the exact same variant name that maintenance-pool uses for a real, live, load-bearing check (contracts/maintenance-pool/src/error.rs:14, constructed at contracts/maintenance-pool/src/lib.rs:130-132 when amount > pool.balance). Escrow's release/refund perform no equivalent check anywhere — neither function verifies the contract's actual on-chain token balance is sufficient before transferring; both simply trust that escrow.amount (recorded once, at fund time) still accurately reflects available funds, and transfer that recorded amount unconditionally. Given maintenance-pool treats "verify sufficient balance before paying out" as an explicit, tested, first-class check, the presence of an identically-named, unused variant in escrow is a plausible signal that an equivalent defensive check was intended for escrow too and never implemented — as opposed to being simply irrelevant scaffolding.

Requirements

  • Determine definitively, via git blame/history if available, whether NotExpired and InsufficientBalance are relics of removed validation logic or were always dead scaffolding that was never wired up.
  • If InsufficientBalance represents a genuinely missing defensive check: add it to release and refund, verifying token::Client::balance(&env.current_contract_address()) >= escrow.amount (or the specific amount about to be transferred) before transferring, returning Error::InsufficientBalance if not. This is a meaningful defense-in-depth addition given #18's solvency-invariant work is about proving balance always covers obligations — this check would make that invariant enforced at the point of payment rather than only verified after the fact by an external fuzzer.
  • If, after investigation, both variants are confirmed to be pure unused scaffolding with no missing check behind them, remove them and document the removal explicitly (contract error enums are part of the public ABI — removing/renumbering variants is a breaking change for any client code matching on specific error codes, so this needs the same care as any other public-interface change, not a silent cleanup).

Acceptance Criteria

  • Investigation into NotExpired/InsufficientBalance's origin documented (even if inconclusive, the reasoning should be recorded)
  • Either: InsufficientBalance wired into release/refund as a genuine defensive balance check, with tests — or: both variants removed with an explicit, documented rationale and a note about the ABI-compatibility implications of removing a contracterror variant
  • If added: test_release_rejects_if_contract_balance_insufficient / test_refund_rejects_if_contract_balance_insufficient (likely requiring a contrived setup — e.g. draining the contract's balance via a means outside normal accounting, or testing the check directly via env.as_contract)
  • cargo test --workspace passes

Additional Notes

  • Precise references: contracts/escrow/src/error.rs:16-17 (the two dead variants, NotExpired = 10, InsufficientBalance = 11); confirmed dead via grep -n "Error::NotExpired\|Error::InsufficientBalance" contracts/escrow/src/lib.rs returning no matches.
  • Cross-references: docs/access-control-audit.md's existing "Error::Unauthorized is dead code (not a bug)" section — this issue explicitly does not claim the same conclusion for these two variants, since (unlike Unauthorized) there's no structural reason (like require_auth()'s panic-not-Result behavior) that would make them permanently unreachable by design; #18 (on-chain solvency invariant fuzzer — complementary: that issue verifies the balance-covers-obligations invariant externally/statistically, this issue is about whether the contract should also assert it inline, at the moment of payment, using the very error variant that already exists for exactly that purpose); maintenance-pool::withdraw's real InsufficientBalance check (contracts/maintenance-pool/src/lib.rs:130-132) as the direct model to follow if the check is added to escrow.
Ngôn ngữ chính
Rust
Star
1
Fork
102
Merge trung bình
4 giờ 12 phút
Pull request đã merge (30 ngày)
30

Chuẩn bị môi trường

  • Không có Dockerfile hay tệp Docker Compose
  • Có mẫu pull request
  • Không có hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của MergeFi/contracts

Tất cả issue của MergeFi/contracts

Issue tương tự

Thêm issue về Rust

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.