Background / Context
Every payout path (escrow::release, escrow::refund, milestones::release_issue, milestones::cancel_milestone, maintenance_pool::withdraw) performs one or more token::Client::transfer calls to externally-supplied or previously-stored Addresses before or interleaved with the contract's own persistent-storage state mutation. For example, in escrow::release (contracts/escrow/src/lib.rs:97-133), token transfers to treasury and each recipient happen at lines 119-126, and escrow.status = EscrowStatus::Paid is only persisted at line 128-129 — after all transfers complete.
Problem Statement
Soroban's execution model differs from EVM's in ways that change (but do not necessarily eliminate) reentrancy risk: cross-contract calls are synchronous, and a malicious token or recipient contract address could, depending on Soroban's authorization/call-stack rules, attempt to call back into release/refund/withdraw mid-execution. Because the status flag (EscrowStatus::Paid, IssueStatus::Released) is written after transfers, a reentrant call during the transfer loop would still see EscrowStatus::Funded and could pass the match escrow.status guard again, potentially double-paying from the same escrow if a reentrant call path exists.
This needs a rigorous, Soroban-specific analysis (not an assumption imported from Ethereum reentrancy folklore) of whether this is actually exploitable given Soroban's host-enforced call semantics, and either a proof it's safe or a fix (checks-effects-interactions reordering: flip status to Paid/Released before performing transfers).
Requirements
- Research and document Soroban's actual cross-contract call/reentrancy model: can a
token::Client::transfer recipient (a custom SEP-41 token or a "recipient" that is itself a contract) execute arbitrary code that calls back into the calling contract during the transfer?
- Build a proof-of-concept malicious contract (recipient or token) that attempts reentrant calls into
release, refund, and withdraw and add it to the test suite to empirically validate or refute the theoretical risk.
- Regardless of the empirical finding, refactor all four payout functions to follow strict checks-effects-interactions ordering (state mutation and persistence before external calls) as defense-in-depth, since this costs little and removes the class of bug entirely.
- Add regression tests proving state is consistent even if a hypothetical reentrant call were possible.
Acceptance Criteria
Technical Notes / Hints
- Files:
contracts/escrow/src/lib.rs (release 97-133, refund 137-170), contracts/milestones/src/lib.rs (release_issue 127-177, cancel_milestone 181-208), contracts/maintenance-pool/src/lib.rs (withdraw 110-151).
- Note
compute_split is called and fully resolved before any transfer in release/release_issue already — the risk is specifically about status persistence timing, not the split math.
Difficulty Justification
Requires platform-specific expertise in Soroban's execution/authorization model (genuinely different from the EVM patterns most reentrancy guidance assumes), building working adversarial contracts in no_std Rust against soroban-sdk, and carefully refactoring five functions across three crates without altering their externally-observable success-case behavior.
Background / Context
Every payout path (
escrow::release,escrow::refund,milestones::release_issue,milestones::cancel_milestone,maintenance_pool::withdraw) performs one or moretoken::Client::transfercalls to externally-supplied or previously-storedAddresses before or interleaved with the contract's own persistent-storage state mutation. For example, inescrow::release(contracts/escrow/src/lib.rs:97-133), token transfers totreasuryand eachrecipienthappen at lines 119-126, andescrow.status = EscrowStatus::Paidis only persisted at line 128-129 — after all transfers complete.Problem Statement
Soroban's execution model differs from EVM's in ways that change (but do not necessarily eliminate) reentrancy risk: cross-contract calls are synchronous, and a malicious
tokenorrecipientcontract address could, depending on Soroban's authorization/call-stack rules, attempt to call back intorelease/refund/withdrawmid-execution. Because the status flag (EscrowStatus::Paid,IssueStatus::Released) is written after transfers, a reentrant call during the transfer loop would still seeEscrowStatus::Fundedand could pass thematch escrow.statusguard again, potentially double-paying from the same escrow if a reentrant call path exists.This needs a rigorous, Soroban-specific analysis (not an assumption imported from Ethereum reentrancy folklore) of whether this is actually exploitable given Soroban's host-enforced call semantics, and either a proof it's safe or a fix (checks-effects-interactions reordering: flip status to
Paid/Releasedbefore performing transfers).Requirements
token::Client::transferrecipient (a custom SEP-41 token or a "recipient" that is itself a contract) execute arbitrary code that calls back into the calling contract during the transfer?release,refund, andwithdrawand add it to the test suite to empirically validate or refute the theoretical risk.Acceptance Criteria
release,refund,release_issue,cancel_milestone,withdrawall reordered to checks-effects-interactionsTechnical Notes / Hints
contracts/escrow/src/lib.rs(release97-133,refund137-170),contracts/milestones/src/lib.rs(release_issue127-177,cancel_milestone181-208),contracts/maintenance-pool/src/lib.rs(withdraw110-151).compute_splitis called and fully resolved before any transfer inrelease/release_issuealready — the risk is specifically about status persistence timing, not the split math.Difficulty Justification
Requires platform-specific expertise in Soroban's execution/authorization model (genuinely different from the EVM patterns most reentrancy guidance assumes), building working adversarial contracts in
no_stdRust againstsoroban-sdk, and carefully refactoring five functions across three crates without altering their externally-observable success-case behavior.