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

# [M-1] Use of Total Player Count Instead of Actual Claimants Leads to Underpayment and Stuck Funds

jck

[M-1] Use of Total Player Count Instead of Actual Claimants Leads to Underpayment and Stuck Funds

Description

  • Normal behavior: Remaining rewards should be distributed equally among all claimants.

  • Issue: The calculation in closePot() divides by i_players.length instead of claimants.length, underpaying claimants and leaving leftover rewards in the contract.

uint256 claimantCut = @> (remainingRewards - managerCut) / i_players.length;

Risk

Likelihood:

  • Occurs whenever not all registered players claim their rewards.

  • Common in contests with inactive or unresponsive players.

Impact:

  • Some rewards remain undistributed.

  • Contract holds leftover funds that cannot be claimed.

Severity: Medium (M)

Proof of Concept

Click to expand PoC
function test_incorrectRedistributionNew() public {
// Use existing setup - no need to redeclare anything
​
// Player1 claims their reward
vm.prank(player1);
pot.claimCut();
​
// Fast forward past deadline
vm.warp(block.timestamp + 91 days);
​
// Owner closes pot (player2 never claimed)
vm.prank(user);
pot.closePot();
​
// Check balances
uint256 player1Balance = weth.balanceOf(player1);
uint256 userBalance = weth.balanceOf(user);
uint256 potRemainingBalance = weth.balanceOf(address(pot));
​
console.log("Player1 balance:", player1Balance);
console.log("User (owner) balance:", userBalance);
console.log("Pot remaining balance:", potRemainingBalance);
​
// Player1 should have their original reward PLUS extra from incorrect redistribution
// Original reward: 3 ETH
// Extra from bug: 0.45 ETH (1 ETH remaining - 10% manager cut = 0.9 ETH / 2 players = 0.45 ETH)
uint256 expectedPlayer1Total = 3 ether + 0.45 ether; // 3.45 ETH
​
assertEq(player1Balance, expectedPlayer1Total, "Player1 should get extra from incorrect redistribution");
​
// Also show the exact math breakdown
console.log("Expected with bug:", expectedPlayer1Total);
console.log("Actual:", player1Balance);
console.log("Difference:", player1Balance - expectedPlayer1Total);
​
// Additional verification: Pot should have leftover funds
assertGt(potRemainingBalance, 0, "Pot should have leftover funds due to incorrect division");
}

Recommended Mitigation

Accurate redistribution:
The original bug divided the leftover rewards by i_players.length (all registered players), rather than only those who did not yet claim. This caused early claimants to be underpaid, leaving leftover funds stranded. The fix counts only unclaimed players using claimableCount.

Pull-based reward distribution:
Instead of sending rewards inside a loop (which can fail due to DoS from malicious or reverting tokens), we record the amount in redistributedRewards for each player. Each player can withdraw individually, making the system safer and gas-efficient.

Single closure enforcement:
The potClosed flag ensures closePot() can only be called once, preventing repeated manager cuts or accidental overpayments.

Accounting correctness:
remainingRewards is explicitly set to 0 after redistribution, ensuring that the contract’s accounting always matches the actual token balance.

Safe ERC20 transfer:
Each withdrawal uses a require() check to ensure that the token transfer succeeds, avoiding silent failures from non-standard ERC20 tokens.

- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
+ // Use pull-based approach and account for unclaimed players
+ mapping(address => uint256) public redistributedRewards;
+ bool public potClosed;
+
+ function closePot() external onlyOwner {
+ require(!potClosed, "Pot already closed");
+ require(block.timestamp - i_deployedAt >= 90 days, "Still open for claim");
+
+ potClosed = true;
+
+ // Manager cut
+ uint256 managerCut = (remainingRewards * managerCutPercent) / 100;
+ require(i_token.transfer(owner(), managerCut), "Manager cut failed");
+
+ // Remaining rewards for unclaimed players
+ uint256 leftover = remainingRewards - managerCut;
+ uint256 claimableCount = 0;
+ for (uint256 i = 0; i < claimants.length; i++) {
+ if (!hasClaimed[claimants[i]]) {
+ claimableCount++;
+ }
+ }
+
+ // Accrue each unclaimed player's share with remainder (dust) handling
+ if (claimableCount > 0) {
+ uint256 share = leftover / claimableCount;
+ uint256 remainder = leftover % claimableCount;
+
+ for (uint256 i = 0; i < claimants.length; i++) {
+ address player = claimants[i];
+ if (!hasClaimed[player]) {
+ uint256 amountToAssign = share;
+ if (remainder > 0) {
+ amountToAssign += 1;
+ remainder--;
+ }
+ redistributedRewards[player] = amountToAssign;
+ }
+ }
+ }
+
+ remainingRewards = 0;
+ }
+
+ // Pull-based withdrawal
+ function withdrawRedistributedReward() external {
+ uint256 amount = redistributedRewards[msg.sender];
+ require(amount > 0, "No reward to withdraw");
+
+ redistributedRewards[msg.sender] = 0;
+ require(i_token.transfer(msg.sender, amount), "Transfer failed");
+ }
Updates

Lead Judging Commences

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