Skip to content

Make bind_settlement_token reject rebinding once a token is already set #644

Description

@mikewheeleer

Make bind_settlement_token reject rebinding once a token is already set

Description

bind_settlement_token in contracts/escrow/src/lib.rs documents a SettlementTokenAlreadyBound error in its # Errors section, but the implementation never checks for an existing binding — write_settlement_token unconditionally overwrites DataKey::SettlementToken. Because every fund movement (deposit, release, refund, cancel, fee withdrawal) reads this single token, an admin (or a compromised admin key) can swap the settlement token mid-lifecycle to a different asset while the contract still holds the original token's balance. That silently strands the original deposits and breaks the one-asset-per-escrow invariant the rest of the code assumes.

This issue makes the binding write-once: bind_settlement_token (and its set_settlement_token alias) must fail with a typed error if a token is already bound, matching the documented contract.

Requirements and context

  • Repository scope: Talenttrust/Talenttrust-Contracts only.
  • Add the SettlementTokenAlreadyBound variant to the canonical Error enum (append-only to preserve client-SDK stability).
  • In bind_settlement_token, panic with that error when read_settlement_token(&env).is_some(), before writing.
  • Ensure the set_settlement_token alias inherits the same guard (it already delegates).
  • Keep the existing NotInitialized and UnauthorizedRole gates and the admin require_auth.

Suggested execution

  • Fork the repo and create a branch
  • git checkout -b security/contracts-settlement-token-write-once
  • Implement changes
    • Write code in: contracts/escrow/src/lib.rs — add the already-bound guard in bind_settlement_token; add the error variant in contracts/escrow/src/types.rs.
    • Write comprehensive tests in: contracts/escrow/src/test/sac_custody.rs — assert the first bind succeeds, a second bind reverts with SettlementTokenAlreadyBound, and the original token remains bound.
    • Add documentation: confirm the write-once semantics in the bind_settlement_token doc comment.
    • Include NatSpec-style doc comments (///) reflecting the new error path.
    • Validate security assumptions: no mid-lifecycle asset swap, no rebind even by the admin.
  • Test and commit

Test and commit

  • Run cargo fmt --all -- --check, cargo build, and cargo test.
  • Cover edge cases and failure paths: rebind by admin, rebind by non-admin, rebind before initialization.
  • Include the full cargo test output and a short security notes section in the PR description.

Example commit message

fix: reject settlement-token rebinding to enforce write-once binding

Guidelines

  • Minimum 95 percent test coverage for impacted modules.
  • Clear, reviewer-focused documentation.
  • Timeframe: 96 hours.

Community & contribution rewards

  • 💬 Join the TalentTrust community on Discord for questions, reviews, and faster merges: https://discord.gg/WqnGpcPx
  • ⭐ This is a GrantFox OSS / Official Campaign task and may be rewarded. When your PR is merged you'll be prompted to rate the project — if this issue and the maintainers helped you ship, we'd be grateful for a 5-star rating. Clear questions in Discord and tidy, well-tested PRs are the fastest path to a merge and a reward.

Metadata

Metadata

Assignees

No one assigned

    Fields

    No fields configured for Feature.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions