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

Incorrect divisor in `closePot()` calculates claimant payouts using total player count instead of actual claimant count, permanently locking unclaimed rewards in the contract

memegod81

Root + Impact

Description

  • Describe the normal behavior in one or more sentences

  • Explain the specific issue or problem in one or more sentences
    closePot() calculates each claimant's share of the remaining reward pool by dividing by i_players.length (the total number of players originally entered into the contest), instead of claimants.length (the number of players who actually called claimCut() before the 90-day claim window closed):

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

  • Impact 1

closePot() calculates each claimant's share of the remaining reward pool by dividing by i_players.length (the total number of players originally entered into the contest), instead of claimants.length (the number of players who actually called claimCut() before the 90-day claim window closed):

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

Since the loop only iterates over claimants (the smaller, actual-claimant set), but the payout amount is calculated using the larger i_players.length, the total amount distributed will always be less than remainingRewards whenever claimants.length < i_players.length — which is the normal, expected case, since not every player is guaranteed to claim within the 90-day window.

Impact:

Any time fewer players claim than the total number originally entered into the contest (the expected, common case), a portion of the reward pool is permanently and irrecoverably left stranded in the Pot contract. For example, with 10 total players, 900 remaining tokens, and only 3 actual claimants: each claimant receives 900 / 10 = 90 tokens, for a total payout of 270 — leaving 630 tokens (70% of the remaining pool) stuck in the contract with no function anywhere in Pot.sol able to retrieve them. This happens automatically, with no attacker or malicious action required — it is guaranteed incorrect behavior in the normal operation of the protocol, harming the very claimants the protocol is meant to reward.

Proof of Concept

Proof of Concept:

function test_closePotStrandsFundsWhenNotAllPlayersClaim() public {
// Contest set up with 10 players, e.g. 900 tokens remaining at close time
// Only 3 of the 10 players call claimCut() before the 90-day window elapses
​
vm.prank(player1);
pot.claimCut();
vm.prank(player2);
pot.claimCut();
vm.prank(player3);
pot.claimCut();
​
vm.warp(block.timestamp + 90 days + 1);
​
uint256 potBalanceBefore = token.balanceOf(address(pot));
​
vm.prank(owner);
pot.closePot();
​
uint256 potBalanceAfter = token.balanceOf(address(pot));
​
// Expected: potBalanceAfter == 0 (all funds distributed)
// Actual: potBalanceAfter > 0 — funds permanently stuck due to incorrect divisor
assert(potBalanceAfter > 0);
}

Recommended Mitigation

Recommendation:

Divide by the actual number of claimants, not the total player count:

- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
+ uint256 claimantCut = claimants.length > 0
+ ? (remainingRewards - managerCut) / claimants.length
+ : 0;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}

Consider also handling any remainder from integer division (e.g., sending dust to the manager or the last claimant) to ensure the contract balance reaches exactly zero after closure.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 24 minutes ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-02] Incorrect logic in `Pot::closePot` leads to unfair distribution to `claimants`, potentially locking the funds with no way to take that out

## Description in `closePot` function while calclulating the shares for claimaint cut, `i_players.length` is used, instead of `claimants.length`, causing low amount being distributed to claimants. ## Vulnerability Details [2024-08-MyCut/src/Pot.sol at main · Cyfrin/2024-08-MyCut (github.com)](https://github.com/Cyfrin/2024-08-MyCut/blob/main/src/Pot.sol#L57) `Pot::closePot` function is meant to be called once contest passed 90 days, it sends the owner cut to owner and rest is splitted among the users who claimed b/w 90 days period. However, current implementation is wrong.&#x20; It uses total users (i_players.length) instead of the users (claimants.length) who claimed during the duration. This creates an unfair distribution to the participants and some of the funds could be locked in the contract. In worst case scenerio, it could be 90% if nobody has claimed from the protocol during the 90 days duration. ## POC In existing test suite, add following test: ```solidity function testUnfairDistributionInClosePot() public mintAndApproveTokens { // Setup address[] memory testPlayers = new address[](3); testPlayers[0] = makeAddr("player1"); testPlayers[1] = makeAddr("player2"); testPlayers[2] = makeAddr("player3"); uint256[] memory testRewards = new uint256[](3); testRewards[0] = 400; testRewards[1] = 300; testRewards[2] = 300; uint256 testTotalRewards = 1000; // Create and fund the contest vm.startPrank(user); address testContest = ContestManager(conMan).createContest( testPlayers, testRewards, IERC20(ERC20Mock(weth)), testTotalRewards ); ContestManager(conMan).fundContest(0); vm.stopPrank(); // Only player1 claims their reward vm.prank(testPlayers[0]); Pot(testContest).claimCut(); // Fast forward 91 days vm.warp(block.timestamp + 91 days); // Record balances before closing the pot uint256 player1BalanceBefore = ERC20Mock(weth).balanceOf( testPlayers[0] ); // Close the contest vm.prank(user); ContestManager(conMan).closeContest(testContest); // Check balances after closing the pot uint256 player1BalanceAfter = ERC20Mock(weth).balanceOf(testPlayers[0]); // Calculate expected distributions uint256 remainingRewards = 600; // 300 + 300 unclaimed rewards uint256 ownerCut = remainingRewards / 10; // 10% of remaining rewards uint256 distributionPerPlayer = (remainingRewards - ownerCut) / 1; // as only 1 user claimed uint256 fundStucked = ERC20Mock(weth).balanceOf(address(testContest)); // actual results console.log("expected reward:", distributionPerPlayer); console.log( "actual reward:", player1BalanceAfter - player1BalanceBefore ); console.log("Fund stucked:", fundStucked); } ``` then run `forge test --mt testUnfairDistributionInClosePot -vv` in the terminal and it will show following output: ```js [⠊] Compiling... [⠒] Compiling 1 files with Solc 0.8.20 [⠘] Solc 0.8.20 finished in 1.63s Compiler run successful! Ran 1 test for test/TestMyCut.t.sol:TestMyCut [PASS] testUnfairDistributionInClosePot() (gas: 905951) Logs: User Address: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D Contest Manager Address 1: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353 Minting tokens to: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D Approved tokens to: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353 expected reward: 540 actual reward: 180 Fund stucked: 360 Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.58ms (506.33µs CPU time) ``` ## Impact Loss of funds, Unfair distribution b/w users ## Recommendations Fix the functions as shown below: ```diff 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; + uint256 totalClaimants = claimants.length; + if(totalClaimant == 0){ + _transferReward(msg.sender, remainingRewards - managerCut); + } else { + uint256 claimantCut = (remainingRewards - managerCut) / claimants.length; for (uint256 i = 0; i < claimants.length; i++) { _transferReward(claimants[i], claimantCut); } } + } } ```

Support

FAQs

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

Give us feedback!