Sending tokens in a for loop

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x8b8b1f5a83a884203e202d43e55b44f7399a774dd193897c6e98a017b51f251e Severity: low

Description: Description

Performing token transfers in a loop in a Solidity contract is generally not recommended due to various reasons. One of these reasons is the "Fail-Silently" issue.

In a Solidity loop, if one transfer operation fails, it causes the entire transaction to fail. This issue can be particularly troublesome when you're dealing with multiple transfers in one transaction. For instance, if you're looping through an array of recipients to distribute dividends or rewards, a single failed transfer will prevent all the subsequent recipients from receiving their transfers. This could be due to the recipient contract throwing an exception or due to other issues like a gas limit being exceeded.

Moreover, such a design could also inadvertently lead to a situation where a malicious contract intentionally causes a failure when receiving Ether to prevent other participants from getting their rightful transfers. This could open up avenues for griefing attacks in the system.

Resolution: To mitigate this problem, it's typically recommended to follow the "withdraw pattern" in your contracts instead of pushing payments. In this model, each recipient would be responsible for triggering their own payment. This separates each transfer operation, so a failure in one doesn't impact the others. Additionally, it greatly reduces the chance of malicious interference as the control over fund withdrawal lies with the intended recipient and not with an external loop operation.

Attachments

1. Proof of Concept (PoC) File

['298']

298:        for (uint256 _i = 0; _i < _bundles.length; _i++) {
299:             ClaimFeeBundle calldata _bundle = _bundles[_i];
300: 
301:             if (_bundle.token == address(0)) revert InvalidToken();
302:             if (_bundle.receiver == address(0)) revert InvalidReceiver();
303: 
304:             uint256 _claimAmount = claimableFees[_bundle.token];
305:             if (_claimAmount == 0) revert ZeroAmount();
306: 
307:             delete claimableFees[_bundle.token];
308:             IERC20(_bundle.token).safeTransfer(_bundle.receiver, _claimAmount); // <= FOUND
309:             emit ClaimFee(_bundle.token, _claimAmount, _bundle.receiver);
310:         }

['298']

298:         for (uint256 _i = 0; _i < _bundles.length; _i++) {
299:             ClaimFeeBundle calldata _bundle = _bundles[_i];
300: 
301:             if (_bundle.token == address(0)) revert InvalidToken();
302:             if (_bundle.receiver == address(0)) revert InvalidReceiver();
303: 
304:             uint256 _claimAmount = claimableFees[_bundle.token];
305:             if (_claimAmount == 0) revert ZeroAmount();
306: 
307:             delete claimableFees[_bundle.token];
308:             IERC20(_bundle.token).safeTransfer(_bundle.receiver, _claimAmount); // <= FOUND
309:             emit ClaimFee(_bundle.token, _claimAmount, _bundle.receiver);
310:         }

['165']

165:        for (uint256 _i = 0; _i < _bundles.length; _i++) {
166:             CreateBundle memory _bundle = _bundles[_i];
167: 
168:             if (_bundle.pool == address(0)) revert InvalidPool();
169:             if (_bundle.from <= block.timestamp) revert InvalidFrom();
170:             if (
171:                 _bundle.to < _bundle.from + _minimumCampaignDuration
172:                     || _bundle.to - _bundle.from > _maximumCampaignDuration
173:             ) revert InvalidTo();
174:             if (
175:                 _bundle.rewardTokens.length == 0 || _bundle.rewardTokens.length > MAX_REWARDS_PER_CAMPAIGN
176:                     || _bundle.rewardTokens.length != _bundle.rewardAmounts.length
177:             ) revert InvalidRewards();
178: 
179:             bytes32 _id = _campaignId(_bundle);
180:             Campaign storage campaign = campaigns[_id];
181:             if (campaign.from != 0) revert CampaignAlreadyExists();
182: 
183:             campaign.owner = msg.sender;
184:             campaign.chainId = _bundle.chainId;
185:             campaign.pool = _bundle.pool;
186:             campaign.from = _bundle.from;
187:             campaign.to = _bundle.to;
188:             campaign.specification = _bundle.specification;
189:             campaign.rewards = _bundle.rewardTokens;
190: 
191:             uint256[] memory _feeAmounts = new uint256[](_bundle.rewardTokens.length);
192:             uint256[] memory _rewardAmountsMinusFees = new uint256[](_bundle.rewardTokens.length);
193:             for (uint256 _j = 0; _j < _bundle.rewardTokens.length; _j++) {
194:                 address _token = _bundle.rewardTokens[_j];
195:                 if (_token == address(0)) revert InvalidRewards();
196: 
197:                 uint256 _amount = _bundle.rewardAmounts[_j];
198:                 if (_amount == 0) revert InvalidRewards();
199: 
200:                 for (uint256 _k = _j + 1; _k < _bundle.rewardTokens.length; _k++) {
201:                     if (_token == _bundle.rewardTokens[_k]) revert InvalidRewards();
202:                 }
203: 
204:                 uint256 _feeAmount = _amount * _fee / UNIT;
205:                 uint256 _rewardAmountMinusFees = _amount - _feeAmount;
206:                 claimableFees[_token] += _feeAmount;
207: 
208:                 _feeAmounts[_j] = _feeAmount;
209:                 _rewardAmountsMinusFees[_j] = _rewardAmountMinusFees;
210: 
211:                 Reward storage reward = campaign.reward[_token];
212:                 reward.amount = _rewardAmountMinusFees;
213:                 reward.unclaimed = _rewardAmountMinusFees;
214: 
215:                 IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount); // <= FOUND
216:             }
217: 
218:             emit CreateCampaign(
219:                 _id,
220:                 msg.sender,
221:                 _bundle.chainId,
222:                 _bundle.pool,
223:                 _bundle.from,
224:                 _bundle.to,
225:                 _bundle.specification,
226:                 _bundle.rewardTokens,
227:                 _rewardAmountsMinusFees,
228:                 _feeAmounts
229:             );
230:         }

['165']

165:         for (uint256 _i = 0; _i < _bundles.length; _i++) {
166:             CreateBundle memory _bundle = _bundles[_i];
167: 
168:             if (_bundle.pool == address(0)) revert InvalidPool();
169:             if (_bundle.from <= block.timestamp) revert InvalidFrom();
170:             if (
171:                 _bundle.to < _bundle.from + _minimumCampaignDuration
172:                     || _bundle.to - _bundle.from > _maximumCampaignDuration
173:             ) revert InvalidTo();
174:             if (
175:                 _bundle.rewardTokens.length == 0 || _bundle.rewardTokens.length > MAX_REWARDS_PER_CAMPAIGN
176:                     || _bundle.rewardTokens.length != _bundle.rewardAmounts.length
177:             ) revert InvalidRewards();
178: 
179:             bytes32 _id = _campaignId(_bundle);
180:             Campaign storage campaign = campaigns[_id];
181:             if (campaign.from != 0) revert CampaignAlreadyExists();
182: 
183:             campaign.owner = msg.sender;
184:             campaign.chainId = _bundle.chainId;
185:             campaign.pool = _bundle.pool;
186:             campaign.from = _bundle.from;
187:             campaign.to = _bundle.to;
188:             campaign.specification = _bundle.specification;
189:             campaign.rewards = _bundle.rewardTokens;
190: 
191:             uint256[] memory _feeAmounts = new uint256[](_bundle.rewardTokens.length);
192:             uint256[] memory _rewardAmountsMinusFees = new uint256[](_bundle.rewardTokens.length);
193:             for (uint256 _j = 0; _j < _bundle.rewardTokens.length; _j++) {
194:                 address _token = _bundle.rewardTokens[_j];
195:                 if (_token == address(0)) revert InvalidRewards();
196: 
197:                 uint256 _amount = _bundle.rewardAmounts[_j];
198:                 if (_amount == 0) revert InvalidRewards();
199: 
200:                 for (uint256 _k = _j + 1; _k < _bundle.rewardTokens.length; _k++) {
201:                     if (_token == _bundle.rewardTokens[_k]) revert InvalidRewards();
202:                 }
203: 
204:                 uint256 _feeAmount = _amount * _fee / UNIT;
205:                 uint256 _rewardAmountMinusFees = _amount - _feeAmount;
206:                 claimableFees[_token] += _feeAmount;
207: 
208:                 _feeAmounts[_j] = _feeAmount;
209:                 _rewardAmountsMinusFees[_j] = _rewardAmountMinusFees;
210: 
211:                 Reward storage reward = campaign.reward[_token];
212:                 reward.amount = _rewardAmountMinusFees;
213:                 reward.unclaimed = _rewardAmountMinusFees;
214: 
215:                 IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount); // <= FOUND
216:             }
217: 
218:             emit CreateCampaign(
219:                 _id,
220:                 msg.sender,
221:                 _bundle.chainId,
222:                 _bundle.pool,
223:                 _bundle.from,
224:                 _bundle.to,
225:                 _bundle.specification,
226:                 _bundle.rewardTokens,
227:                 _rewardAmountsMinusFees,
228:                 _feeAmounts
229:             );
230:         }

['193']

193:             for (uint256 _j = 0; _j < _bundle.rewardTokens.length; _j++) {
194:                 address _token = _bundle.rewardTokens[_j];
195:                 if (_token == address(0)) revert InvalidRewards();
196: 
197:                 uint256 _amount = _bundle.rewardAmounts[_j];
198:                 if (_amount == 0) revert InvalidRewards();
199: 
200:                 for (uint256 _k = _j + 1; _k < _bundle.rewardTokens.length; _k++) {
201:                     if (_token == _bundle.rewardTokens[_k]) revert InvalidRewards();
202:                 }
203: 
204:                 uint256 _feeAmount = _amount * _fee / UNIT;
205:                 uint256 _rewardAmountMinusFees = _amount - _feeAmount;
206:                 claimableFees[_token] += _feeAmount;
207: 
208:                 _feeAmounts[_j] = _feeAmount;
209:                 _rewardAmountsMinusFees[_j] = _rewardAmountMinusFees;
210: 
211:                 Reward storage reward = campaign.reward[_token];
212:                 reward.amount = _rewardAmountMinusFees;
213:                 reward.unclaimed = _rewardAmountMinusFees;
214: 
215:                 IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount); // <= FOUND
216:             }

[luzzif]

I get the issue, but also, being that the client builds the claim bundles, if multiple are used at the same time and one fails, it's also possible to exclude the failing claim from the array and proceed with all the others, or go about it one-by one (the claim rewards function can be called with one bundle at a time, and no loop is involved there).