Validate deposit_funds inputs before moving tokens to prevent transfer-then-revert
Description
The public deposit_funds in contracts/escrow/src/lib.rs calls token_client.transfer(&caller, &env.current_contract_address(), &amount) first, and only afterward delegates to deposit::deposit_funds_impl, which is where amount > 0, caller == client, ContractNotFound, and status == Created are validated. This ordering is wrong: a negative amount will reach the token transfer before AmountMustBePositive is checked, an unknown contract_id triggers a host token call before ContractNotFound, and a non-client caller's require_auth/role check happens only after funds have already been pulled. The settlement token is also read with .expect("Settlement token not set"), a host panic rather than a typed error.
This issue moves all validation ahead of the transfer so the contract never pulls tokens for a call that will revert, and replaces the expect with a typed SettlementTokenNotConfigured error.
Requirements and context
- Repository scope: Talenttrust/Talenttrust-Contracts only.
- Load the contract, verify it exists, verify
caller == contract.client, run caller.require_auth(), verify status == Created, and verify amount > 0 — all before any token::Client::transfer.
- Replace
read_settlement_token(...).expect(...) with a typed error path returning SettlementTokenNotConfigured.
- Keep
deposit_funds_impl as the single source of accounting truth; refactor so validation is not duplicated between the wrapper and the impl.
- Preserve the
Funded promotion behavior and any deposit event semantics.
Suggested execution
- Fork the repo and create a branch
git checkout -b security/contracts-deposit-validate-before-transfer- Implement changes
- Write code in:
contracts/escrow/src/lib.rs — reorder validation before the transfer; thread checks through deposit::deposit_funds_impl in contracts/escrow/src/deposit.rs.
- Write comprehensive tests in:
contracts/escrow/src/test/deposit.rs — assert that negative-amount, wrong-caller, and unknown-contract deposits revert with no token movement (balance unchanged).
- Add documentation: note the validate-before-transfer ordering in the
deposit_funds doc comment.
- Include NatSpec-style doc comments (
///) on the changed entrypoint.
- Validate security assumptions: no token pull on any reverting path, unconfigured token yields a typed error.
- Test and commit
Test and commit
- Run
cargo fmt --all -- --check, cargo build, and cargo test.
- Cover edge cases and failure paths: zero/negative amount, non-client caller, unknown contract id, unset settlement token.
- Include the full
cargo test output and a short security notes section in the PR description.
Example commit message
fix: validate deposit_funds inputs before token transfer and use a typed token-unset error
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.
Validate deposit_funds inputs before moving tokens to prevent transfer-then-revert
Description
The public
deposit_fundsincontracts/escrow/src/lib.rscallstoken_client.transfer(&caller, &env.current_contract_address(), &amount)first, and only afterward delegates todeposit::deposit_funds_impl, which is whereamount > 0,caller == client,ContractNotFound, andstatus == Createdare validated. This ordering is wrong: a negativeamountwill reach the token transfer beforeAmountMustBePositiveis checked, an unknowncontract_idtriggers a host token call beforeContractNotFound, and a non-client caller'srequire_auth/role check happens only after funds have already been pulled. The settlement token is also read with.expect("Settlement token not set"), a host panic rather than a typed error.This issue moves all validation ahead of the transfer so the contract never pulls tokens for a call that will revert, and replaces the
expectwith a typedSettlementTokenNotConfigurederror.Requirements and context
caller == contract.client, runcaller.require_auth(), verifystatus == Created, and verifyamount > 0— all before anytoken::Client::transfer.read_settlement_token(...).expect(...)with a typed error path returningSettlementTokenNotConfigured.deposit_funds_implas the single source of accounting truth; refactor so validation is not duplicated between the wrapper and the impl.Fundedpromotion behavior and any deposit event semantics.Suggested execution
git checkout -b security/contracts-deposit-validate-before-transfercontracts/escrow/src/lib.rs— reorder validation before the transfer; thread checks throughdeposit::deposit_funds_implincontracts/escrow/src/deposit.rs.contracts/escrow/src/test/deposit.rs— assert that negative-amount, wrong-caller, and unknown-contract deposits revert with no token movement (balance unchanged).deposit_fundsdoc comment.///) on the changed entrypoint.Test and commit
cargo fmt --all -- --check,cargo build, andcargo test.cargo testoutput and a short security notes section in the PR description.Example commit message
fix: validate deposit_funds inputs before token transfer and use a typed token-unset errorGuidelines
Community & contribution rewards