escrow: dead NotExpired/InsufficientBalance error variants suggest a missing defensive balance check in release()/refund()
维护者通常 1 天内回复
评估
这个 Issue 还没有评估数据。
描述
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:
NotExpiredreads exactly like the error a deadline-gated function would return when called before its deadline — i.e., precisely the shape of checkrefund's early-path used to need before it was refactored to the currentif 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 attemptingrefundbefore expiry was rejected with a typedNotExpirederror rather than relying onrequire_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.InsufficientBalanceis the exact same variant name thatmaintenance-pooluses for a real, live, load-bearing check (contracts/maintenance-pool/src/error.rs:14, constructed atcontracts/maintenance-pool/src/lib.rs:130-132whenamount > pool.balance). Escrow'srelease/refundperform no equivalent check anywhere — neither function verifies the contract's actual on-chain token balance is sufficient before transferring; both simply trust thatescrow.amount(recorded once, atfundtime) still accurately reflects available funds, and transfer that recorded amount unconditionally. Givenmaintenance-pooltreats "verify sufficient balance before paying out" as an explicit, tested, first-class check, the presence of an identically-named, unused variant inescrowis 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
NotExpiredandInsufficientBalanceare relics of removed validation logic or were always dead scaffolding that was never wired up. - If
InsufficientBalancerepresents a genuinely missing defensive check: add it toreleaseandrefund, verifyingtoken::Client::balance(&env.current_contract_address()) >= escrow.amount(or the specific amount about to be transferred) before transferring, returningError::InsufficientBalanceif 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:
InsufficientBalancewired intorelease/refundas 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 acontracterrorvariant - 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 viaenv.as_contract) -
cargo test --workspacepasses
Additional Notes
- Precise references:
contracts/escrow/src/error.rs:16-17(the two dead variants,NotExpired = 10,InsufficientBalance = 11); confirmed dead viagrep -n "Error::NotExpired\|Error::InsufficientBalance" contracts/escrow/src/lib.rsreturning no matches. - Cross-references:
docs/access-control-audit.md's existing "Error::Unauthorizedis dead code (not a bug)" section — this issue explicitly does not claim the same conclusion for these two variants, since (unlikeUnauthorized) there's no structural reason (likerequire_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 realInsufficientBalancecheck (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 模板
- 没有贡献指南
从这里开始
- 先读完整个 Issue,再读项目的贡献指南。
- 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
- Fork 仓库,在一个分支上完成修改。
- 提交 Pull Request,并在描述里引用这个 Issue 编号。
MergeFi/contracts 的其他 Issue
-
allocations map retains released issues — phantom ledger drift可能重新可做 @PINYOPATTANAWASANPORN 于 12 天前认领,目前没有进行中的 PR。 未关闭
难度 2/5 1-3 小时 新手友好度 25/100
MergeFi/contracts#608 · 3 条评论 ·
维护者通常 1 天内回复
-
escrow::contribute and milestones::contribute accept new funds after deadline + GRACE_PERIOD, when anyone can already refund or cancel可能已有人在做 @urielit1313-creator 于 29 天前认领。 未关闭bug
难度 3/5 1-2 天 新手友好度 25/100
MergeFi/contracts#568 · 3 条评论 ·
维护者通常 1 天内回复
-
Makefile deploy targets deploy the unoptimised .wasm — `stellar contract optimize` writes *.optimized.wasm可能已有人在做 @urielit1313-creator 于 29 天前认领。 未关闭bug good first issue
难度 2/5 1-3 小时 新手友好度 25/100
MergeFi/contracts#567 · 1 条评论 ·
维护者通常 1 天内回复
-
keep_alive never refreshes instance storage, so a contract kept alive only via keep_alive can still archive Admin/Oracle/Treasury可能已有人在做 @urielit1313-creator 于 29 天前认领。 未关闭bug
难度 4/5 3-5 天 新手友好度 25/100
MergeFi/contracts#566 · 3 条评论 ·
维护者通常 1 天内回复
-
[Regression on closed #254] milestones::deallocate still has no test asserting admin authorization可能已有人在做 @urielit1313-creator 于 29 天前认领。 未关闭security testing
难度 2/5 1-3 小时 新手友好度 25/100
MergeFi/contracts#565 · 1 条评论 ·
维护者通常 1 天内回复
查看 MergeFi/contracts 的全部 Issue
相似的 Issue
-
难度 2/5 1-3 小时 新手友好度 72/100
aws/aws-emulator#30 · 1 条评论 ·
-
area:post bug effort:low P2 source:agent
难度 1/5 1-3 小时 新手友好度 82/100
维护者通常 1 天内回复
-
难度 2/5 1-3 小时 新手友好度 76/100
维护者通常 1 天内回复
-
Chat images never show without ImageMagick: ffprobe fallback uses an option string ffprobe 9 rejects未关闭
难度 2/5 1-3 小时 新手友好度 79/100
eliasstravik/herdr-projects#1011 ·
-
design-data-spec
难度 2/5 1-3 小时 新手友好度 78/100
adobe/spectrum-design-data#1744 ·
维护者通常 1 天内回复