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

[chonilius]

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 Build an on-chain solvency invariant fuzzer: contract token balance must always cover all outstanding obligations #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; Build an on-chain solvency invariant fuzzer: contract token balance must always cover all outstanding obligations #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.

[Devadakene]

I would like to work on this issue as I am experienced in implementing similar issues

[grantfox-oss]

🦊 GrantFox — @Devadakene has been assigned to this issue as part of the Third Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #48)
2. Your PR will be reviewed by the MergeFi maintainers

Good luck! Track your progress on GrantFox.