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

closePot loops over an unbounded claimants array — a large pot can never be closed, freezing the manager cut and redistribution

hopeh

Description

closePot() pays the post-window redistribution by looping over the entire claimants array and doing one external ERC20 transfer per iteration:

uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut); // external i_token.transfer per claimant
}

claimants grows by one every time a player calls claimCut(), and its upper bound is the number of players the owner places in the pot (i_players), which is unbounded — a pot can be created with an arbitrarily large player list. Each loop iteration is a full ERC20 transfer. Once enough players have claimed, the single closePot transaction that must iterate the whole claimants array exceeds the block gas limit and reverts every time.

closePot is the ONLY function that takes the manager cut and releases the post-window redistribution. If it can never fit in a block, the pot can never be closed: the manager cut and the entire leftover pool are frozen with no alternative path to release them.

Risk

Impact: Medium. A pot with a large claimant set becomes impossible to close, permanently freezing the manager cut and the leftover redistribution (a denial-of-service that ends in locked funds). Unlike a normal revert, there is no smaller/partial close path to fall back to.

Likelihood: Medium. It requires a large player/claimant count, which the owner sets at pot creation — entirely plausible for a sizeable contest or airdrop-style distribution, and not something users can avoid once they've claimed into the array.

Proof of Concept

function test_closePotUnclosableWithManyClaimants() public {
uint256 N = 5000; // large but realistic contest size
address[] memory players = _players(N);
uint256[] memory rewards = _rewards(N, 1e18);
address potAddr = manager.createContest(players, rewards, IERC20(token), N * 1e18);
Pot pot = Pot(potAddr);
token.mint(owner, N * 1e18);
vm.prank(owner); token.approve(address(manager), N * 1e18);
vm.prank(owner); manager.fundContest(0);
​
// Everyone claims -> claimants.length == N.
for (uint256 i = 0; i < N; i++) { vm.prank(players[i]); pot.claimCut(); }
​
vm.warp(block.timestamp + 90 days + 1);
// closePot must iterate N claimants in one tx; with N large it exceeds the block gas limit.
vm.expectRevert(); // out-of-gas: pot can never be closed
vm.prank(owner); manager.closeContest(potAddr);
}

Expected: the pot can always be closed. Actual: past a threshold claimant count the close transaction cannot fit in a block, so the manager cut and leftover are frozen.

Recommended Mitigation

Replace the push-to-everyone loop with a pull-payment pattern: on closePot, record each in-time claimant's post-window share (e.g. mapping(address => uint256) postWindowShare) and let each claimant withdraw it in their own transaction, so no single call has to iterate the whole array:

function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) revert Pot__StillOpenForClaim();
if (remainingRewards == 0) return;
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
// record per-claimant share; do NOT loop-transfer here
perClaimantShare = (remainingRewards - managerCut) / claimants.length; // also fixes the denominator
closed = true;
}
​
function withdrawShare() external {
require(closed && claimed[msg.sender] == false && _isClaimant(msg.sender));
claimed[msg.sender] = true;
_transferReward(msg.sender, perClaimantShare);
}

This bounds every transaction to O(1) work regardless of claimant count. (Alternatively, cap the number of players per pot.)

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 1 hour 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!