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

Incorrect divisor in Pot.closePot (i_players.length instead of claimants.length) permanently locks remaining rewards

tray198

Root cause + Impact

Pot::closePot splits the leftover reward pool among the players who claimed in time, but it divides by i_players.length (the total number of registered players) instead of claimants.length (the number who actually claimed). Whenever this distribution branch runs, claimants.length is strictly smaller than i_players.length, so every in-time claimant is under-paid and the undistributed remainder is stranded in the Pot permanently (there is no withdraw/rescue function). This is an unrecoverable loss of funds.

Description

  • Normal behavior: after the 90-day claim window, closePot takes the 10% manager cut and distributes the whole remaining pool equally among the claimants (the players who claimed in time), leaving the Pot empty aside from rounding dust.

  • The issue: the per-claimant share is computed with the total player count as the divisor while the payout loop iterates over claimants. The contract therefore pays out claimants.length * ((remainingRewards - managerCut) / i_players.length), which is far less than the intended remainingRewards - managerCut. The shortfall stays locked in the contract forever.

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; // BUG: divisor should be claimants.length
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

Risk

Likelihood:

  • Occurs on every pot where at least one registered player does not claim within the 90-day window, which is precisely the situation closePot exists to handle.

  • When all players claim, remainingRewards == 0 and this branch is skipped, so whenever the branch runs the divisor i_players.length is guaranteed larger than claimants.length.

Impact:

  • A large fraction of the leftover pool is never distributed and is permanently locked in the Pot (no withdraw or rescue function exists) — direct, unrecoverable loss of user funds.

  • In-time claimants receive materially less than their intended equal share of the remainder.

Proof of Concept

Pot with 10 players, totalRewards = 1000 (100 each). Only 2 players claim in time, so remainingRewards = 800.

// Inside closePot():
managerCut = 800 / 10 = 80; // sent to manager
claimantCut = (800 - 80) / 10 = 72; // BUG: divides by i_players.length (10)
// loop runs over the 2 claimants:
distributed = 2 * 72 = 144;
​
// Correct (divide by claimants.length == 2):
claimantCut = (800 - 80) / 2 = 360;
distributed = 2 * 360 = 720;
​
// Permanently locked = 720 - 144 = 576 tokens (72% of the post-cut remainder)

The 576 tokens remain trapped in the Pot with no way to retrieve them.

Recommended Mitigation

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

Also guard against claimants.length == 0 (e.g. send the full remainder to the manager when nobody claimed) to avoid a divide-by-zero.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 33 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!