Remove deferred claim flow from escrow payouts

[Schwartz10]

Summary

• remove deferred claim accounting and pay refund/slash recipients immediately
• delete the obsolete claim script and claim-related read output
• update unit and invariant tests for immediate payout semantics and tighter lifecycle invariants

Testing

• forge test

[Schwartz10]

@claude review

[claude]

Claude finished @Schwartz10's task in 2m 4s —— View job

---

Code Review

• Read CLAUDE.md for repo guidelines
• Read changed files and understand the diff
• Analyze the implementation for correctness, security, and best practices
• Post review findings

---

The core change is straightforward and well-executed: replacing the two-step pull flow (accumulate claimAmounts → claim()) with direct pushes inside refundEscrow/slashEscrow. The simplification is real — less state, fewer transactions, cleaner semantics. Below are my findings.

---

Critical: Storage Layout Shift (Upgrade Safety)

GitBondEscrow is a UUPS upgradeable proxy. Removing claimAmounts and deprecatedFlatRefundFee from the Layout struct shifts every subsequent field's storage slot:

Before                                         After
─────────────────────────────────────────      ─────────────────────────────────────────
slot 0  mapping escrows                        slot 0  mapping escrows
slot 1  mapping tokenWhitelist                 slot 1  mapping tokenWhitelist
slot 2  mapping totalEscrowedByToken           slot 2  mapping totalEscrowedByToken
slot 3  mapping claimAmounts          ← gone   slot 3  address treasury          (was 4)
slot 4  address treasury                       slot 4  uint256 platformCutBps    (was 5)
slot 5  uint256 platformCutBps                 slot 5  uint256 minEscrowAmount   (was 7)
slot 6  uint256 deprecatedFlatRefundFee← gone  slot 6  uint256 totalEscrowed     (was 8)
slot 7  uint256 minEscrowAmount                slot 7  mapping refundDelegates   (was 9)
slot 8  uint256 totalEscrowed                  ...
slot 9  mapping refundDelegates
...

If an on-chain proxy is upgraded to this implementation, treasury, platformCutBps, minEscrowAmount, totalEscrowed, and both delegate mappings will read/write to wrong slots. This is likely caught by the OpenZeppelin upgrades plugin — if you're not already running forge script with --ffi and the upgrades plugin, run forge build with the OZ upgrades safety checker before deploying.

If this is a pre-deployment redesign and no proxy is live, this is a non-issue — but worth confirming explicitly.

Fix this →

---

Significant: Push Model Introduces a New DoS Vector

The old claim() pull pattern was resilient by design — a recipient who can't receive tokens is their own problem. The new push pattern means a non-receivable recipient blocks settlement entirely.

Specifically in slashEscrow (src/GitBondEscrow.sol:213-216):

if (platformCut > 0) {
    _transferWithAmountCheck(e.token, s.treasury, platformCut);  // ← runs first
}
_transferWithAmountCheck(e.token, e.counterparty, amountToCounterparty);

If treasury is blacklisted on the token (USDC/USDT have blocklists), every single slash reverts until the owner calls setTreasury. For counterparty, a malicious actor could set up their escrow counterparty as a contract that reverts on receive, making the bond permanently unslashable.

The treasury risk is the more acute one since it's a global shared dependency. Consider whether a treasury transfer failure should allow slashing to proceed with the cut locked in a recoverable reserve, or at minimum document this as an operational requirement for treasury address selection.

---

Minor: Missing Zero-Amount Guard for amountToCounterparty

_transferWithAmountCheck(e.token, e.counterparty, amountToCounterparty) is called unconditionally (src/GitBondEscrow.sol:216), unlike the platformCut branch which has if (platformCut > 0). If platformCutBps were ever set to 10000 (the setPlatformCut cap is 3000, so this can't happen), amountToCounterparty would be 0 and the transfer would be a no-op. Not a real bug given the current cap, but symmetry with the platformCut guard would be cleaner.

---

Positive Notes

• CEI pattern is preserved: both refundEscrow and slashEscrow update all state (e.status, totalEscrowed, totalEscrowedByToken) before making any external calls. No reentrancy risk introduced despite the calls moving earlier.
• Invariant test improvements are excellent: invariant_tokenBalanceConservationHolds is a strong addition — it proves no tokens are minted or burned through the contract. targetSelector tightens the fuzzing surface meaningfully.
• Test renames are semantic and accurate: test_refundEscrowTransfersFullAmountToBeneficiaryImmediately, test_refundAndSlashDistributeToDistinctRecipients, etc. clearly express the new behavior.
• Removing EscrowClaimed event: If you have any off-chain indexers (subgraph, etc.) consuming this event, they'll need updating. Worth verifying no indexers depend on it before deploying.

---

Summary: The implementation is correct and the tests are thorough. The storage layout shift is the one thing that needs verification against deployment state before merging. The push-model DoS surface is worth a conscious decision on whether to accept or mitigate.