Reject a zero or contract-self destination address in withdraw_protocol_fees

[mikewheeleer]

Reject a zero or contract-self destination address in withdraw_protocol_fees

Description

withdraw_protocol_fees(amount, to) in contracts/escrow/src/lib.rs decrements AccumulatedProtocolFees and then transfers amount to the caller-supplied to address with no validation of that destination. If to is the contract's own address (env.current_contract_address()), the fee accounting is reduced while the tokens cycle back into the contract balance — the fees are accounted as withdrawn but remain commingled with escrowed deposits, corrupting the released + refunded + accumulated_fees <= funded_amount invariant the release path depends on. There is no guard preventing this self-transfer or other obviously-wrong destinations.

This issue adds destination validation so accumulated fees can only leave the contract to an external address, keeping the accumulated-fee accounting honest.

Requirements and context

• Repository scope: Talenttrust/Talenttrust-Contracts only.
• Reject to == env.current_contract_address() with a typed error (e.g. InvalidWithdrawalDestination), added append-only to the Error enum.
• Perform the destination check before mutating AccumulatedProtocolFees, so a rejected withdrawal leaves accounting untouched.
• Keep all existing guards: initialization, pause/emergency, admin auth, positive amount, sufficient accumulated fees, and configured settlement token.
• Document that fee withdrawal must target an external treasury address, not the escrow contract itself.

Suggested execution

• Fork the repo and create a branch
• git checkout -b security/contracts-fee-withdraw-destination-guard
• Implement changes
  • Write code in: contracts/escrow/src/lib.rs — add the destination guard in withdraw_protocol_fees; add the error variant in contracts/escrow/src/types.rs.
  • Write comprehensive tests in: contracts/escrow/src/test/protocol_fees.rs — assert a self-destination withdrawal reverts and leaves AccumulatedProtocolFees unchanged, and a valid external withdrawal succeeds.
  • Add documentation: note the external-destination requirement in the entrypoint docs.
  • Include NatSpec-style doc comments (///) on the new error path.
  • Validate security assumptions: no self-transfer drains accounting, invariant preserved.
• Test and commit

Test and commit

• Run cargo fmt --all -- --check, cargo build, and cargo test.
• Cover edge cases and failure paths: self-destination, valid external destination, withdrawal exceeding accrued fees.
• Include the full cargo test output and a short security notes section in the PR description.

Example commit message

fix: reject self-destination in withdraw_protocol_fees to protect fee accounting

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.

[GBOYEE]

Hey! The missing destination guard in withdraw_protocol_fees is a real problem -- if someone passes the escrow contract's own address as to, the fee accounting gets reduced but the tokens just cycle back into the contract balance, breaking the released + refunded + accumulated_fees <= funded_amount invariant.

I'd add an InvalidWithdrawalDestination variant to the Error enum in contracts/escrow/src/types.rs and check to != env.current_contract_address() at the top of withdraw_protocol_fees in contracts/escrow/src/lib.rs before any state mutation. Then write tests in contracts/escrow/src/test/protocol_fees.rs covering self-destination revert + accounting unchanged, and a valid external withdrawal succeeding.

Mind if I take this on? I'll have the PR up within 24 hours.