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

未关闭
#48 2 条评论 0 个 reaction 已指派 1 人 在 GitHub 查看

维护者通常 1 天内回复

@Devadakene 已经在做这个了。

开始于 2026年8月17日。

  • #70 来自 @Devadakene —— 未关闭

评估

这个 Issue 还没有评估数据。

描述

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.
主要语言
Rust
星标
1
派生
102
平均合并
4 小时 12 分钟
30 天内合并 PR
30

环境准备

  • 没有 Dockerfile 或 Docker Compose 文件
  • 有 Pull Request 模板
  • 没有贡献指南

从这里开始

  1. 先读完整个 Issue,再读项目的贡献指南。
  2. 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
  3. Fork 仓库,在一个分支上完成修改。
  4. 提交 Pull Request,并在描述里引用这个 Issue 编号。

MergeFi/contracts 的其他 Issue

查看 MergeFi/contracts 的全部 Issue

相似的 Issue

更多 Rust Issue

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。