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

closePot() splits the remainder across all players instead of only the claimants — under-pays claimants and permanently locks funds

sub99

Summary

In Pot.closePot() the post-deadline remainder is divided by i_players.length (all players) instead of claimants.length (only those who claimed in time). Every in-time claimant is under-paid and the undistributed difference is permanently locked in the Pot, which exposes no withdraw function.

Vulnerability Details

The README states the remainder must be "distributed equally to those who claimed in time", but closePot() uses the wrong denominator:

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

i_players.length counts ALL players, but the loop pays only the claimants. Because closePot() exists precisely to handle rewards that went unclaimed, claimants.length is always < i_players.length on this branch (remainingRewards > 0). So each claimant receives less than their fair share, claimantCut * claimants.length < (remainingRewards - managerCut), and the shortfall is left behind. There is no withdraw/rescue and remainingRewards is never reset, so the stranded balance is unrecoverable.

Proof of Concept

The following Foundry test passes against the in-scope code. Four players are each owed 100 (total 400); only p1 and p2 claim in time (remaining 200); then the owner closes the contest:

function test_closePot_underpays_claimants_and_strands_funds() public {
// owner creates + funds a Pot of 400 over [p1, p2, p3, p4] (100 each)
vm.prank(p1); Pot(pot).claimCut(); // +100
vm.prank(p2); Pot(pot).claimCut(); // +100 -> remainingRewards = 200
vm.warp(block.timestamp + 90 days + 1);
vm.prank(owner); cm.closeContest(pot);
​
// managerCut = 200/10 = 20 ; claimantCut = (200-20)/i_players.length(4) = 45
// CORRECT = (200-20)/claimants.length(2) = 90 each
assertEq(weth.balanceOf(p1), 100 + 45); // under-paid: got 45, owed 90
assertEq(weth.balanceOf(p2), 100 + 45);
assertEq(weth.balanceOf(pot), 90); // 90 tokens permanently stranded
}

Result: each claimant receives 45 instead of 90, and 90 tokens are permanently locked in the Pot with no recovery path.

Impact

Permanent loss of user funds. Every contest close under-pays the honest, in-time claimants and strands the remainder forever. The larger the gap between total players and actual claimants — i.e. the exact situation closePot() exists to handle — the larger the loss; with zero in-time claimants, ~90% of the pool is stranded because the distribution loop runs zero iterations. Likelihood is high: this occurs on the normal close path, with no attacker or special precondition required.

Recommended Mitigation

Divide by the number of claimants, guard the empty case, and reset state so the close cannot re-run:

uint256 claimantCut = claimants.length == 0
? 0
: (remainingRewards - managerCut) / claimants.length;
// ... distribute to claimants ...
remainingRewards = 0;

Send any rounding dust to the manager, and add a rescue/sweep for residual balance.

Tools Used

Foundry (proof-of-concept test), manual review.

Updates

Lead Judging Commences

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