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

Unclaimed remainder is divided by the total player count i_players.length instead of the actual claimant count claimants.length, underpaying every timely claimant and permanently locking the rest inside the Pot

sillyrobot796

Root + Impact

Description

  • Normally, per the README, "the remainder is distributed equally to those who claimed in time" — i.e. the unclaimed remainder (after the manager cut) should be split evenly among claimants.length addresses.

  • The implementation divides by the total number of players instead. Whenever at least one player fails to claim within 90 days, each claimant's share shrinks by a factor of claimants.length / i_players.length, and the un-swept difference stays in the pot forever (no withdrawal path exists).

// src/Pot.sol
function closePot() external onlyOwner {
...
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
@> uint256 claimantCut = (remainingRewards - managerCut) / i_players.length; // divisor should be claimants.length
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

Risk

Likelihood:

  • Any contest where at least one player misses the 90-day claim window triggers the loss — players abandoning an ended contest (lost wallets, unaware winners, etc.) is a routine outcome, not an edge case.

  • The divisor i_players.length is always >= claimants.length and they are only equal when all players claim, i.e. precisely when remainingRewards == 0 and the whole block is skipped — so the bug fires in every situation where the branch executes meaningfully.

Impact:

  • In the standard flow (total 1000, rewards [500,500], one claimant): the claimant receives 225 instead of the specified 450 — a 50% loss — and the other 225 is permanently locked in the Pot.

  • The loss scales with abandonment: with 1 of 10 players claiming, that claimant receives only 81 of the 810 they are owed (10% of entitlement) while 729 remains frozen.

Proof of Concept

PoC explanation: The test quantifies the underpayment in the standard flow. Setup: a 2-player pot ([500, 500]) funded with 1000 WETH. Step 1: player1 claims 500, leaving remainingRewards = 500; player2 misses the window, so exactly one claimant exists. Step 2: after 91 days the admin closes the pot; the remainder is split as managerCut = 50 and claimantCut = (500 - 50) / i_players.length = 225 — even though player1 is the ONLY claimant and, per the README, should receive the full 450. Assertions: p1CloseGain is exactly 225 (a 50% underpayment of the specified 450) and 225 WETH sits permanently on the pot (no sweep exists). The divisor i_players.length counts players who never claimed, so the fire condition is any abandonment at all. Run with: forge test --match-test test_POC2_WrongDivisor_PlayersInsteadOfClaimants -vv (PASS; asserts 225 and the 225 locked balance).

// forge test --match-test test_POC2_WrongDivisor_PlayersInsteadOfClaimants -> PASS
function test_POC2_WrongDivisor_PlayersInsteadOfClaimants() public {
(address[] memory players, uint256[] memory rewards) = _twoPlayers();
address pot = _createAndFund(players, rewards, 1000);
​
vm.prank(p1);
Pot(pot).claimCut(); // p1 +500, remainingRewards = 500
​
vm.warp(91 days);
vm.prank(admin);
conMan.closeContest(pot);
​
uint256 p1CloseGain = weth.balanceOf(p1) - 500;
assertEq(p1CloseGain, 225, "claimant got 225, should be 450 (if /claimants.length)");
assertEq(weth.balanceOf(pot), 225, "225 permanently locked in Pot");
}

Recommended Mitigation

// src/Pot.sol
- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
+ if (claimants.length == 0) {
+ // full remainder to payout recipient — MUST be added together,
+ // otherwise this fix turns the (normal) zero-claimant case into Panic(0x12)
+ i_token.transfer(payoutRecipient, remainingRewards);
+ remainingRewards = 0;
+ return;
+ }
+ uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;

⚠ Combination note: swapping the divisor alone introduces a new division-by-zero panic in the perfectly normal "nobody claimed" scenario — this fix MUST ship atomically with the empty-claimants fallback (V3), and the payout recipient fix (V1), otherwise the redirected remainder lands in the non-withdrawable ContestManager. With V4 still unfixed, each repeated close re-runs the (now correct) division and still over-pays claimants out of non-claimers' funds.

Updates

Lead Judging Commences

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