MyCut
AI First Flight #8
Beginner FriendlyFoundry
EXP
View results
Previous Next
Submission Details
Severity: high
Valid

External calls inside loop in closePot enables DoS and prevents all reward distribution

wanzablock

Root + Impact

Description

  • Describe the normal behavior in one or more sentences

  • The normal behavior is that when a pot closes, all claimants who previously claimed receive an equal share of remaining rewards

  • Explain the specific issue or problem in one or more sentences

  • The closePot() function makes external token transfers inside a loop when distributing claimant cuts.

// Root cause in the codebase with @> marks to highlight the relevant section
// Pot.sol - Lines 49-62
function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
// @audit External call inside loop
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut); // External ERC20 transfer
}
}
}
​
// Pot.sol - Lines 64-66
function _transferReward(address player, uint256 reward) internal {
i_token.transfer(player, reward); // @audit External call
}

Risk

Likelihood:

  • Reason 1 // Describe WHEN this will occur (avoid using "if" statements)

  • Medium-High - This occurs when:

    • Any claimant is a contract with malicious receive/fallback

    • Any claimant is blacklisted on token contract

  • Reason 2

Impact:

  • Impact 1

  • High Impact:

    1. DoS Attack: Single malicious claimant can prevent all distributions permanently

    2. Gas Limit: Contest with 200+ claimants cannot close due to gas limits

  • Impact 2

Proof of Concept

// Attack Scenario 1: Malicious claimant DoS
contract MaliciousPlayer {
fallback() external payable {
revert("DoS attack"); // Always revert on token receipt
}
}
​
// 1. MaliciousPlayer claims their cut successfully via claimCut()
// 2. 90 days pass, 10,000 tokens remain
// 3. Manager calls closePot()
// 4. Manager receives 1,000 tokens (10%)
// 5. Loop starts distributing to claimants
// 6. When MaliciousPlayer's turn comes, transfer reverts
// 7. Entire closePot() transaction reverts
// 8. 9,000 tokens locked forever
// 9. Other honest claimants cannot receive their share
​
// Attack Scenario 2: Gas limit DoS
// 1. Contest has 500 claimants
// 2. Each transfer costs ~50,000 gas
// 3. Total gas needed: 25,000,000
// 4. Block gas limit: 30,000,000
// 5. closePot() consumes all gas and reverts
// 6. Contest can never close
// 7. All remaining rewards locked

Recommended Mitigation

- remove this code
+ add this code// Pot.sol
contract Pot is Ownable(msg.sender) {
+ mapping(address => uint256) public closeRewards;
+ bool public potClosed;
+ uint256 public closeTimestamp;
​
function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
+ require(!potClosed, "Already closed");
+ potClosed = true;
+ closeTimestamp = block.timestamp;
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.safeTransfer(msg.sender, managerCut);
+ // Calculate rewards for each claimant but don't transfer
uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
- for (uint256 i = 0; i < claimants.length; i++) {
- _transferReward(claimants[i], claimantCut);
- }
+ for (uint256 i = 0; i < claimants.length; i++) {
+ closeRewards[claimants[i]] = claimantCut;
+ }
}
}
​
+ // New function: Users claim their close rewards
+ function claimCloseReward() external {
+ require(potClosed, "Pot not closed yet");
+ uint256 reward = closeRewards[msg.sender];
+ require(reward > 0, "No rewards");
+
+ closeRewards[msg.sender] = 0;
+ i_token.safeTransfer(msg.sender, reward);
+ }
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 3 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-04] Gas Limit DoS via large amount of claimants

## Description The `Pot.sol` contract contains a vulnerability that can lead to a Denial of Service (DoS) attack. This issue arises from the inefficient handling of claimants in the `closePot` function, where iterating over a large number of claimants can cause the transaction to run out of gas, thereby preventing the contract from executing as intended. ## Vulnerability Details Affected code - <https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L58> The vulnerability is located in the `closePot` function of the Pot contract, specifically at the loop iterating over the claimants array: ```javascript function closePot() external onlyOwner { ... if (remainingRewards > 0) { ... @> for (uint256 i = 0; i < claimants.length; i++) { _transferReward(claimants[i], claimantCut); } } } ``` The `closePot` function is designed to distribute remaining rewards to claimants after a contest ends. However, if the number of claimants is extremly large, the loop iterating over the claimants array can consume a significant amount of gas. This can lead to a situation where the transaction exceeds the gas limit and fails, effectively making it impossible to close the pot and distribute the rewards. ## Exploit 1. Attacker initiates a big contest with a lot of players 2. People claim the cut 3. Owner closes the large pot that will be very costly ```javascript function testGasCostForClosingPotWithManyClaimants() public mintAndApproveTokens { // Generate 2000 players address[] memory players2000 = new address[](2000); uint256[] memory rewards2000 = new uint256[](2000); for (uint256 i = 0; i < 2000; i++) { players2000[i] = address(uint160(i + 1)); rewards2000[i] = 1 ether; } // Create a contest with 2000 players vm.startPrank(user); contest = ContestManager(conMan).createContest(players2000, rewards2000, IERC20(ERC20Mock(weth)), 2000 ether); ContestManager(conMan).fundContest(0); vm.stopPrank(); // Allow 1500 players to claim their cut for (uint256 i = 0; i < 1500; i++) { vm.startPrank(players2000[i]); Pot(contest).claimCut(); vm.stopPrank(); } // Fast forward time to allow closing the pot vm.warp(91 days); // Record gas usage for closing the pot vm.startPrank(user); uint256 gasBeforeClose = gasleft(); ContestManager(conMan).closeContest(contest); uint256 gasUsedClose = gasBeforeClose - gasleft(); vm.stopPrank(); console.log("Gas used for closing pot with 1500 claimants:", gasUsedClose); } ``` ```Solidity Gas used for closing pot with 1500 claimants: 6425853 ``` ## Impact The primary impact of this vulnerability is a Denial of Service (DoS) attack vector. An attacker (or even normal usage with a large number of claimants) can cause the `closePot` function to fail due to excessive gas consumption. This prevents the distribution of remaining rewards and the execution of any subsequent logic in the function, potentially locking funds in the contract indefinitely. In the case of smaller pots it would be a gas inefficency to itterate over the state variabel `claimants`. ## Recommendations Gas Optimization: Optimize the loop to reduce gas consumption by using a local variable to itterate over, like in the following example: ```diff - for (uint256 i = 0; i < claimants.length; i++) { - _transferReward(claimants[i], claimantCut); - } + uint256 claimants_length = claimants.length; + ... + for (uint256 i = 0; i < claimants_length; i++) { + _transferReward(claimants[i], claimantCut); + } ``` Batch Processing: Implement batch processing for distributing rewards. This will redesign the protocol functionallity but instead of processing all claimants in a single transaction, allow the function to process a subset of claimants per transaction. This can be achieved by introducing pagination or limiting the number of claimants processed in one call. This could also be fixed if the user would claim their reward after 90 days themselves

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!