feat: recovery of excess funds#180
Conversation
📝 WalkthroughWalkthroughThe PR implements per-token and per-Ether accounting in swap contracts to track locked amounts for outstanding swaps. The contracts now inherit from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
contracts/test/ERC20SwapFuzzTest.sol (1)
184-205: Consider also covering the refund path in the lifecycle test.The test name
testLockedAmountsTracksLifecyclesuggests full lifecycle coverage, but only the lock→claim path is exercised. Adding a symmetric test (or a branch via a bool fuzz parameter) that verifieslockedAmounts[token]is decremented back to0afterrefund/refundCooperativewould round out the accounting guarantees introduced by this PR — which is central to issue#177(excess = balance − lockedAmounts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/ERC20SwapFuzzTest.sol` around lines 184 - 205, Extend testLockedAmountsTracksLifecycle to also exercise the refund path: add a boolean fuzz parameter (e.g., bool useRefund) or a separate symmetric test that after locking (using preimageHash, amount, token, claimAddress, timelock) either executes the existing claim flow OR advances time past timelock and calls swap.refund (or the cooperative variant swap.refundCooperative) with the same parameters; then assert swap.lockedAmounts(address(token)) == 0 and assertLe(swap.lockedAmounts(address(token)), token.balanceOf(address(swap))). Use the same setup variables (preimage/preimageHash, amount, timelock, token, claimAddress) and vm.warp as needed to move past timelock before calling refund.contracts/EtherSwap.sol (2)
536-624: Consider wrapping thelockedEtherdecrements inuncheckedfor consistency and gas.Same reasoning as for
ERC20Swap:checkSwapIsLockedensures the swap being claimed/refunded previously contributedamounttolockedEther, so the decrement cannot underflow. This aligns with the existingunchecked { toSend += swapAmount; }pattern used inclaimBatchon the same hot paths.♻️ Proposed diff
delete swaps[hash]; - // Decrement the accounted locked Ether - lockedEther -= amount; + // Decrement the accounted locked Ether. + // Safe: checkSwapIsLocked guarantees this swap contributed `amount` to lockedEther. + unchecked { + lockedEther -= amount; + }Apply to all three decrement sites (
prepareCommitmentClaim,prepareClaim,refundInternal).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/EtherSwap.sol` around lines 536 - 624, Wrap the three decrements of lockedEther in an unchecked block to mirror the ERC20Swap optimization and save gas: locate the three sites in prepareCommitmentClaim (if present), prepareClaim, and refundInternal where "lockedEther -= amount;" is executed, and change them to perform the subtraction inside an unchecked { ... } block (you can rely on checkSwapIsLocked having validated the swap existed so underflow cannot occur). Ensure you modify all three occurrences for consistency.
366-372: Minor:sweepTokenwill revert unhelpfully ontoken == address(0).
IERC20(address(0)).balanceOf(address(this))triggers a low-level call to the zero address, which returns no data and causes the ABI decode to revert without a clear error. Since this isonlyOwnerit's not a security issue, but a cheap explicit guard (or relying onNoExcessby short-circuiting) would give a better failure mode for mis-typed calldata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/EtherSwap.sol` around lines 366 - 372, sweepToken currently calls IERC20(token).balanceOf(...) which will revert confusingly when token == address(0); add an explicit guard at the start of sweepToken to require(token != address(0)) (or similarly revert with a clear error) before calling IERC20.balanceOf, then proceed with the existing balance check using NoExcess, emit SweptToken, and call TransferHelper.safeTransferToken; reference sweepToken, IERC20(token).balanceOf, NoExcess, and TransferHelper.safeTransferToken when making the change.contracts/ERC20Swap.sol (1)
614-674: Consider wrapping thelockedAmountsdecrements inuncheckedfor consistency and gas.The invariant
lockedAmounts[tokenAddress] >= amountat these sites is guaranteed bycheckSwapIsLocked(hash)(the swap must have been locked, contributing itsamountexactly once to the accumulator). This is the same reasoning used for the existingunchecked { toSend += swapAmount; }blocks inclaimBatch. Usinguncheckedhere matches that pattern and removes the per-claim/refund overflow check on a hot path.♻️ Proposed diff
delete swaps[hash]; - // Decrement the accounted locked amount for this token - lockedAmounts[tokenAddress] -= amount; + // Decrement the accounted locked amount for this token. + // Safe: checkSwapIsLocked guarantees this swap contributed `amount` to lockedAmounts. + unchecked { + lockedAmounts[tokenAddress] -= amount; + }Apply the same change to the other two decrement sites (
prepareClaimandrefundInternal).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/ERC20Swap.sol` around lines 614 - 674, Wrap the decrement of lockedAmounts in an unchecked block to skip the redundant overflow check (same pattern as the existing unchecked in claimBatch). In prepareClaim and refundInternal replace the direct subtraction of lockedAmounts[tokenAddress] -= amount with an unchecked block performing that subtraction (ensure the compiler version supports unchecked). This keeps the invariant enforced by checkSwapIsLocked(hash) while saving gas on the hot path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/ERC20Swap.sol`:
- Around line 614-674: Wrap the decrement of lockedAmounts in an unchecked block
to skip the redundant overflow check (same pattern as the existing unchecked in
claimBatch). In prepareClaim and refundInternal replace the direct subtraction
of lockedAmounts[tokenAddress] -= amount with an unchecked block performing that
subtraction (ensure the compiler version supports unchecked). This keeps the
invariant enforced by checkSwapIsLocked(hash) while saving gas on the hot path.
In `@contracts/EtherSwap.sol`:
- Around line 536-624: Wrap the three decrements of lockedEther in an unchecked
block to mirror the ERC20Swap optimization and save gas: locate the three sites
in prepareCommitmentClaim (if present), prepareClaim, and refundInternal where
"lockedEther -= amount;" is executed, and change them to perform the subtraction
inside an unchecked { ... } block (you can rely on checkSwapIsLocked having
validated the swap existed so underflow cannot occur). Ensure you modify all
three occurrences for consistency.
- Around line 366-372: sweepToken currently calls IERC20(token).balanceOf(...)
which will revert confusingly when token == address(0); add an explicit guard at
the start of sweepToken to require(token != address(0)) (or similarly revert
with a clear error) before calling IERC20.balanceOf, then proceed with the
existing balance check using NoExcess, emit SweptToken, and call
TransferHelper.safeTransferToken; reference sweepToken, IERC20(token).balanceOf,
NoExcess, and TransferHelper.safeTransferToken when making the change.
In `@contracts/test/ERC20SwapFuzzTest.sol`:
- Around line 184-205: Extend testLockedAmountsTracksLifecycle to also exercise
the refund path: add a boolean fuzz parameter (e.g., bool useRefund) or a
separate symmetric test that after locking (using preimageHash, amount, token,
claimAddress, timelock) either executes the existing claim flow OR advances time
past timelock and calls swap.refund (or the cooperative variant
swap.refundCooperative) with the same parameters; then assert
swap.lockedAmounts(address(token)) == 0 and
assertLe(swap.lockedAmounts(address(token)), token.balanceOf(address(swap))).
Use the same setup variables (preimage/preimageHash, amount, timelock, token,
claimAddress) and vm.warp as needed to move past timelock before calling refund.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98c496f8-20a5-4856-a429-1720d69ab392
📒 Files selected for processing (12)
contracts/ERC20Swap.solcontracts/ERC20SwapTimestamp.solcontracts/EtherSwap.solcontracts/EtherSwapTimestamp.solcontracts/script/Deploy.solcontracts/test/ERC20SwapFuzzTest.solcontracts/test/ERC20SwapTest.solcontracts/test/ERC20SwapTimestampTest.solcontracts/test/EtherSwapFuzzTest.solcontracts/test/EtherSwapTest.solcontracts/test/EtherSwapTimestampTest.solcontracts/test/RouterTestBase.sol
1 participant
Closes #177
Summary by CodeRabbit
Release Notes
New Features
Chores