fix: security hardening - CEI pattern, over-withdrawal guard, debt settlement

[giwaov]

Summary

This PR addresses three security vulnerabilities in RefundProtocol.sol:

1. Reentrancy risk in _executeRefund (fixes #12)

The _executeRefund\ function performed an external \iatToken.transfer()\ call before updating \payments[paymentID].refunded = true. This violates the checks-effects-interactions (CEI) pattern and opens a reentrancy vector if the token contract is malicious or upgradeable.

Fix: Move the state update (\payments[paymentID].refunded = true) before the external transfer call.

2. Over-withdrawal via repeated earlyWithdrawByArbiter calls (fixes #11)

The \earlyWithdrawByArbiter\ function checks \withdrawalAmount > payment.amount\ per payment but never validates the cumulative withdrawn amount. An arbiter could call this function multiple times to withdraw more than the payment amount.

Fix: Add a cumulative check: \payment.withdrawnAmount + withdrawalAmount > payment.amount.

3. Debt bypass in refundByRecipient (fixes #10)

The
efundByRecipient\ function does not call _settleDebt(msg.sender)\ before checking and deducting the recipient's balance, unlike \withdraw()\ which does settle debt first. This means a recipient with outstanding debt can get a full refund without settling.

Fix: Add _settleDebt(msg.sender)\ call before the balance check, matching the pattern in \withdraw().

Testing

All 36 existing tests pass:

\
forge test -vv

36 passed, 0 failed, 0 skipped

\\

Changes

• \src/RefundProtocol.sol: +7/-1 lines across three targeted fixes

[Cassxbt]

This guard looks like the right fix for #11. I think it should be locked down with a regression test though, because the failure mode is specifically about pooled recipient balance, not only a single payment balance.

A good test case:

• create payment A: 100 USDC to receiver
• create payment B: 100 USDC to the same receiver
• call earlyWithdrawByArbiter([paymentA], [60], ...)
• call it again for paymentA with a different valid salt/signature and another 60
• assert the second call reverts with InvalidWithdrawalAmount

Without this guard, payment A reaches withdrawnAmount = 120 and consumes balance that belongs to payment B because balances[receiver] is shared. The code fix is correct; the regression test would prove the exact invariant this PR is restoring.

[giwaov]

Thanks, added a regression test for this case.

It creates two payments to the same receiver, withdraws 60 from the first payment, then tries another signed 60 withdrawal against that same payment using a different salt. The second call now reverts with InvalidWithdrawalAmount, so the shared receiver balance cannot be used to over-withdraw payment A.

Verified with:

• forge test --match-test testEarlyWithdrawByArbiterCannotOverWithdrawPaymentWithSharedRecipientBalance
• forge test