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

Abierto
  • ¿Algo no está bien?
#48 2 comentarios 0 reacciones 1 asignado Ver en GitHub

Los mantenedores suelen responder en 1 día

@Devadakene ya está trabajando en esto.

Desde el 17/8/2026.

  • #70 de @Devadakene — abierto

Evaluación

Este issue todavía no se ha evaluado.

Descripción

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.
Lenguaje dominante
Rust
Estrellas
1
Forks
102
Merge medio
4 h 12 min
PR fusionados (30 d)
30

Preparar el entorno

  • Sin Dockerfile ni archivo de Docker Compose
  • Tiene una plantilla de pull request
  • Sin guía de contribución

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de MergeFi/contracts

Todos los issues de MergeFi/contracts

Issues similares

Más issues de Rust

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.