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

Wrong denominator in closePot distributes less than intended to claimants

sharonkosgei4

Root + Impact

Description

  • When a Pot is closed after the 90-day claim window, closePot is meant to distribute the post-manager-cut remainder equally among the players who claimed in time. Under normal operation, the manager calls closeContest , which triggers closePot, which sends 10% of the remaining balance to the manager and splits the rest among claimants.

  • The bug is that the claimant cut is calculated by dividing by i_players.length the total number of registered players instead of claimants.length the number of players who actually called claimCut within the window. Whenever some players do not claim, the per-claimant share is under-calculated, and the difference is permanently locked in the Pot contract with no recovery path.

// src/Pot.sol
​
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);
​
// @> Bug: divides by ALL registered players, not just those who claimed
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
// @> Loop only iterates over actual claimants — mismatch with divisor above
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

Risk

Likelihood:

  • Any time a contest closes with fewer claimants than registered players, the remaining tokens after distribution are permanently stranded in the Pot. This is the expected common case since not all players in a contest will always claim.

  • There is no admin function to recover stranded tokens, making every such close a permanent loss.

Impact:

  • Claimants receive a fraction of what they are owed. With N registered players and K claimants (K < N), each claimant only receives 1/N of the remainder instead of 1/K, losing (N - K) / N of the post-manager-cut pool.

  • The undelivered tokens are locked in the Pot forever — there is no withdraw or sweep function to recover them.

Proof of Concept

Add the following test to test/PocTests.t.sol and run forge test --match-test test -vvv:

function test_WrongDenominatorLocksClaimantFunds() public {
address[] memory players = new address[](3);
players[0] = player1;
players[1] = player2;
players[2] = player3;
​
uint256[] memory rewards = new uint256[](3);
rewards[0] = 300 ether;
rewards[1] = 300 ether;
rewards[2] = 400 ether;
​
// Owner creates and funds the contest
vm.startPrank(owner);
address contestAddr = conMan.createContest(players, rewards, IERC20(address(weth)), TOTAL_REWARDS);
conMan.fundContest(0);
vm.stopPrank();
​
// Only player1 claims within the 90-day window
vm.prank(player1);
Pot(contestAddr).claimCut();
​
// Remaining rewards = 1000 - 300 = 700 WETH
assertEq(Pot(contestAddr).getRemainingRewards(), 700 ether);
​
uint256 player1BalanceBeforeClose = weth.balanceOf(player1);
​
vm.warp(block.timestamp + 91 days);
​
// closePot sends managerCut to ContestManager (the Pot owner), not the EOA
uint256 conManBalanceBefore = weth.balanceOf(address(conMan));
​
vm.prank(owner);
conMan.closeContest(contestAddr);
​
uint256 conManBalanceAfter = weth.balanceOf(address(conMan));
uint256 player1BalanceAfterClose = weth.balanceOf(player1);
uint256 potBalanceAfterClose = weth.balanceOf(contestAddr);
​
// Manager correctly receives 70 WETH (10% of 700)
uint256 managerReceived = conManBalanceAfter - conManBalanceBefore;
assertEq(managerReceived, 70 ether, "manager should receive 70 WETH");
​
// BUG: player1 receives 210 WETH instead of the correct 630 WETH
// claimantCut = (700 - 70) / i_players.length(3) = 210 ← bug
// correct = (700 - 70) / claimants.length(1) = 630
uint256 player1Bonus = player1BalanceAfterClose - player1BalanceBeforeClose;
assertEq(player1Bonus, 210 ether, "bug: player1 only gets 210 WETH due to wrong divisor");
assertTrue(player1Bonus < 630 ether, "player1 receives less than owed");
​
// BUG: 420 WETH are permanently locked in the Pot with no recovery path
assertEq(potBalanceAfterClose, 420 ether, "420 WETH permanently locked in contract");
}

Recommended Mitigation

Replace i_players.length with claimants.length as the divisor, and add a zero-claimants guard (which also fixes the other finding):

- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
- for (uint256 i = 0; i < claimants.length; i++) {
- _transferReward(claimants[i], claimantCut);
- }
+ if (claimants.length > 0) {
+ uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
+ for (uint256 i = 0; i < claimants.length; i++) {
+ _transferReward(claimants[i], claimantCut);
+ }
+ } else {
+ // No claimants — return remainder to manager
+ i_token.transfer(msg.sender, remainingRewards - managerCut);
+ }
Updates

Lead Judging Commences

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