MyCut

## 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