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

closePot() distributes the remaining rewards using the wrong denominator and leaves funds stranded

ranktang

Root + Impact

Description

  • When the 90-day claim window ends, closePot() is supposed to distribute the leftover rewards to players who claimed in time. However, the leftover amount per claimant is computed by dividing by i_players.length (the total number of players) instead of claimants.length (the players who actually claimed).

    As a result, the leftover pool is not fully distributed. A significant portion remains stuck in the Pot contract, and the pot cannot be properly closed in a single call. The owner must call closePot() repeatedly, but each call still uses the wrong divisor and also pays the manager cut again from the shrinking remainder, so the pot drains unevenly and leaves dust behind each time.

  • The dividend is the leftover pool (remainingRewards - managerCut), which is meant for claimants only, but the divisor is i_players.length, which includes players who never claimed.

  • remainingRewards is never decreased after distribution, so the contract keeps thinking it still holds the full leftover amount.

Even if i_players.length were correct in some sense, the loop only iterates over claimants.length, so the total paid out is:
claimants.length * ((remainingRewards - managerCut) / i_players.length)

which is less than (remainingRewards - managerCut) whenever claimants.length < i_players.length. The difference stays in the contract.

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

Risk

Likelihood:

  • High. This happens any time at least one player does not claim before the 90-day window ends. That is the normal expected scenario the mechanism is designed for.


Impact:

  • The remaining rewards are not fully distributed to the players who claimed in time.

  • A large portion of the pool remains locked in the Pot contract indefinitely.

  • Calling closePot() again does not help cleanly:

    • remainingRewards is unchanged, so the same wrong math is applied again.

    • The manager gets another cut from the same pool on each call.

    • Each call distributes a smaller remainder, leaving dust.

  • The stated protocol behavior ("the remainder is distributed equally to those who claimed in time") is violated.

  • The pot can never be properly closed with a single call, and the owner must effectively repeat the process until the leftover is negligible.

Proof of Concept

Setup:

  • players = [player1, player2]

  • rewards = [500, 500]

  • totalRewards = 1000

  • managerCutPercent = 10

Steps:

  1. player1 claims 500.

  2. player2 never claims.

  3. After 90 days, owner calls closePot().

At this point remainingRewards = 500 and claimants = [player1].

managerCut = 500 / 10 = 50
claimantCut = (500 - 50) / 2 = 225

player1 receives 225, the manager receives 50, and 225 remains stuck in the contract — confirmed by the test below.

function testUnclaimedRewardDistribution() public mintAndApproveTokens {
vm.startPrank(user);
rewards = [500, 500];
totalRewards = 1000;
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
​
uint256 claimantBalanceBefore = ERC20Mock(weth).balanceOf(player1);
​
vm.warp(91 days);
​
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
​
uint256 claimantBalanceAfter = ERC20Mock(weth).balanceOf(player1);
​
assert(claimantBalanceAfter > claimantBalanceBefore);
​
uint256 remaingRewardsAfterClosingPot = ERC20Mock(weth).balanceOf(contest);
assert(remaingRewardsAfterClosingPot == 225);
​
console.log(remaingRewardsAfterClosingPot);
}

Recommended Mitigation

Use claimants.length as the divisor so the leftover pool is split only among players who actually claimed. Also, zero out remainingRewards (or set it to the correct leftover) after the distribution to prevent repeated calls from re-paying and re-cutting the same pool.

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 distributable = remainingRewards - managerCut;
+ uint256 claimantCut = distributable / claimants.length;
+
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
+
+ // Prevent double distribution on repeated calls
+ remainingRewards = 0;
}
}
Updates

Lead Judging Commences

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