diff --git a/src/core/PaymentsRewards.sol b/src/core/PaymentsRewards.sol index 96fe845..1a1a7ee 100644 --- a/src/core/PaymentsRewards.sol +++ b/src/core/PaymentsRewards.sol @@ -17,6 +17,9 @@ abstract contract PaymentsRewards is Base { using EnumerableSet for EnumerableSet.AddressSet; event RewardsClaimed(address indexed account, address indexed token, uint256 amount); + /// @notice Emitted when `claimRewardsAll` skips a token whose transfer reverted, so + /// indexers and the affected operator can see which token is griefing the sweep. + event RewardsClaimSkipped(address indexed account, address indexed token); // ═══════════════════════════════════════════════════════════════════════════ // REWARDS @@ -43,15 +46,47 @@ abstract contract PaymentsRewards is Base { } } - /// @notice Claim pending rewards for all tokens tracked for the caller + /// @notice Claim pending rewards for all tokens tracked for the caller. + /// @dev Each token is claimed in an isolated self-call so a single griefing ERC20 + /// (transfer reverts, gas bomb, paused, etc.) cannot brick the entire sweep + /// and lock out the operator's other reward claims. Tokens whose self-call + /// reverts remain in the pending-rewards set for a future retry once the + /// issue is resolved off-chain. function claimRewardsAll() external nonReentrant { EnumerableSet.AddressSet storage set = _pendingRewardTokens[msg.sender]; - while (set.length() > 0) { - address token = set.at(set.length() - 1); - _claimRewardsToken(msg.sender, token, true); + uint256 len = set.length(); + // Snapshot first: the inner claim mutates the set on success. + address[] memory toClaim = new address[](len); + for (uint256 i = 0; i < len;) { + toClaim[i] = set.at(i); + unchecked { + ++i; + } + } + for (uint256 i = 0; i < len;) { + try this._claimRewardsTokenSafe(msg.sender, toClaim[i]) { + // pending zeroed, transfer succeeded, token removed from set inside the self-call + } catch { + // griefing / paused / reverting token: skip this one, leave it in the set, + // and continue sweeping the remaining tokens + emit RewardsClaimSkipped(msg.sender, toClaim[i]); + } + unchecked { + ++i; + } } } + /// @notice Self-call entry point for `claimRewardsAll`'s per-token try/catch. + /// @dev External so the parent `claimRewardsAll` can wrap each invocation in + /// `try/catch`; gated by `msg.sender == address(this)` so external callers + /// cannot hit it directly. Not `nonReentrant` because the parent already holds + /// the guard for the whole sweep. + function _claimRewardsTokenSafe(address account, address token) external { + if (msg.sender != address(this)) revert Errors.Unauthorized(); + _claimRewardsToken(account, token, true); + } + /// @notice Get pending rewards function pendingRewards(address account) external view returns (uint256) { return _pendingRewards[account][address(0)]; diff --git a/src/facets/tangle/TanglePaymentsRewardsFacet.sol b/src/facets/tangle/TanglePaymentsRewardsFacet.sol index 28989fe..1fca033 100644 --- a/src/facets/tangle/TanglePaymentsRewardsFacet.sol +++ b/src/facets/tangle/TanglePaymentsRewardsFacet.sol @@ -10,7 +10,7 @@ import { IFacetSelectors } from "../../interfaces/IFacetSelectors.sol"; /// escrow funding, and distribution selectors live on `TanglePaymentsFacet`. contract TanglePaymentsRewardsFacet is PaymentsRewards, IFacetSelectors { function selectors() external pure returns (bytes4[] memory selectorList) { - selectorList = new bytes4[](13); + selectorList = new bytes4[](14); selectorList[0] = bytes4(keccak256("claimRewards()")); selectorList[1] = bytes4(keccak256("claimRewards(address)")); selectorList[2] = bytes4(keccak256("claimRewardsBatch(address[])")); @@ -24,5 +24,6 @@ contract TanglePaymentsRewardsFacet is PaymentsRewards, IFacetSelectors { selectorList[10] = this.treasury.selector; selectorList[11] = this.getServiceEscrow.selector; selectorList[12] = this.getBillableServices.selector; + selectorList[13] = this._claimRewardsTokenSafe.selector; } } diff --git a/test/tangle/PaymentEdgeCases.t.sol b/test/tangle/PaymentEdgeCases.t.sol index 8478762..22ba646 100644 --- a/test/tangle/PaymentEdgeCases.t.sol +++ b/test/tangle/PaymentEdgeCases.t.sol @@ -55,6 +55,21 @@ contract RevertingToken is ERC20 { } } +/// @notice Mock token whose `transfer` (outbound) reverts but `transferFrom` (inbound) works. +/// @dev Lets a test set up a pending-rewards entry whose later claim attempt will revert, +/// exercising griefing-resistance on `claimRewardsAll`. +contract OutboundRevertToken is ERC20 { + constructor() ERC20("OutboundRevert", "OREV") { } + + function mint(address to, uint256 amount) external { + _mint(to, amount); + } + + function transfer(address, uint256) public pure override returns (bool) { + revert("Outbound transfer disabled"); + } +} + /// @notice Receiver that rejects ETH contract ETHRejecter { receive() external payable { @@ -223,6 +238,81 @@ contract PaymentEdgeCasesTest is BaseTest { ); } + /// @notice `claimRewardsAll` must isolate per-token transfer failures so a single griefing + /// ERC20 in an operator's pending-rewards set cannot lock out every other claim. + /// @dev Pays the operator in two tokens: a normal `MockERC20` and an `OutboundRevertToken` + /// whose `transfer` reverts. After `claimRewardsAll`: + /// - the normal token's reward is delivered + /// - the griefing token's pending balance is preserved + /// - the griefing token remains in `rewardTokens(operator)` for a future retry + /// - `RewardsClaimSkipped` is emitted for the griefing token + function test_ClaimRewardsAll_SkipsGriefingTokenAndCompletesRest() public { + // Use a uniform 100% operator split so the math is direct. + vm.prank(admin); + tangle.setPaymentSplit( + Types.PaymentSplit({ developerBps: 0, protocolBps: 0, operatorBps: 10_000, stakerBps: 0, keeperBps: 0 }) + ); + + OutboundRevertToken griefToken = new OutboundRevertToken(); + + uint256 normalPay = 7 ether; + uint256 griefPay = 5 ether; + + // Mint + approve the grief token so the user can pay with it via transferFrom. + // OutboundRevertToken inherits ERC20.transferFrom, so request-time inbound works. + griefToken.mint(user1, griefPay); + vm.prank(user1); + griefToken.approve(address(tangle), griefPay); + + // Pay 1: normal MockERC20 (also requires transferFrom path). + token.mint(user1, normalPay); + vm.prank(user1); + token.approve(address(tangle), normalPay); + + address[] memory operators = new address[](1); + operators[0] = operator1; + address[] memory callers = new address[](0); + + vm.prank(user1); + uint64 normalReq = tangle.requestService( + blueprintId, operators, "", callers, 0, address(token), normalPay, Types.ConfidentialityPolicy.Any + ); + _approveService(operator1, normalReq); + + vm.prank(user1); + uint64 griefReq = tangle.requestService( + blueprintId, operators, "", callers, 0, address(griefToken), griefPay, Types.ConfidentialityPolicy.Any + ); + _approveService(operator1, griefReq); + + // Both tokens now sit in the operator's pending-rewards set; no transfer has fired yet. + assertEq(tangle.pendingRewards(operator1, address(token)), normalPay, "normal token pending pre-claim"); + assertEq(tangle.pendingRewards(operator1, address(griefToken)), griefPay, "grief token pending pre-claim"); + assertEq(tangle.rewardTokens(operator1).length, 2, "both tokens tracked"); + + // The sweep: the grief token's transfer reverts, the try/catch wrapper swallows the + // failure, the skip event is emitted, and the normal token is paid out. + vm.recordLogs(); + vm.prank(operator1); + tangle.claimRewardsAll(); + + // Normal token paid. + assertEq(token.balanceOf(operator1), normalPay, "normal token paid out"); + assertEq(tangle.pendingRewards(operator1, address(token)), 0, "normal token cleared from pending"); + + // Grief token preserved for retry. + assertEq(tangle.pendingRewards(operator1, address(griefToken)), griefPay, "grief token pending preserved"); + address[] memory remaining = tangle.rewardTokens(operator1); + assertEq(remaining.length, 1, "only grief token remains in set"); + assertEq(remaining[0], address(griefToken), "grief token retained for retry"); + + // A subsequent direct claim of the grief token should still revert (semantics preserved + // for single-token claims — the user is choosing that specific failure path). + vm.prank(operator1); + vm.expectRevert(); + tangle.claimRewards(address(griefToken)); + } + function test_Payment_RequestServiceWithRevertingTokenReverts() public { RevertingToken revertToken = new RevertingToken(); revertToken.mint(user1, 10 ether);