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

Incorrect distribution denominator causes 80%+ fund lockup in closePot

wanzablock

Root + Impact

Description

  • Describe the normal behavior in one or more sentences

  • The normal behavior should be: after 90 days, remaining funds are distributed equally among players who actually claimed their rewards.

  • Explain the specific issue or problem in one or more sentences

  • The closePot() function distributes remaining rewards by dividing by the total number of registered players (i_players.length) instead of the actual number of claimants (claimants.length).

// Root cause in the codebase with @> marks to highlight the relevant section
// Pot.sol - Lines 49-62
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);
// @audit CRITICAL BUG: Wrong denominator
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length; // Line 56
// @audit Only iterates through claimants (not all players)
for (uint256 i = 0; i < claimants.length; i++) { // Line 57
_transferReward(claimants[i], claimantCut);
}
}
}

Risk

Likelihood:

  • Reason 1 // Describe WHEN this will occur (avoid using "if" statements)

  • High - This occurs in EVERY pot closure where not all players claim their rewards. Given that some players will inevitably miss the 90-day claim window, this is essentially guaranteed to happen.

    Common scenarios:

    • Players forget to claim

    • Players lose access to wallets

  • Reason 2

Impact:

  • Impact 1

  • Massive fund lockup: 80-95% of redistributable funds permanently locked

  • Unfair to active users: Claimants receive 5-20x less than intended

  • No recovery mechanism: Locked funds cannot be retrieved

  • Impact 2

Proof of Concept

// Scenario: 10 registered players, only 2 claim
function testCriticalDistributionBug() public {
// Setup: Create pot with 10 players
address[] memory players = new address[](10);
uint256[] memory rewards = new uint256[](10);
for (uint i = 0; i < 10; i++) {
players[i] = address(uint160(i + 1));
rewards[i] = 100; // 100 tokens each
}
// Total: 1,000 tokens
pot = new Pot(players, rewards, token, 1000);
token.transfer(address(pot), 1000);
// Only 2 players claim (player[0] and player[1])
vm.prank(players[0]);
pot.claimCut(); // Claims 100 tokens
vm.prank(players[1]);
pot.claimCut(); // Claims 100 tokens
// Remaining: 800 tokens
// Fast forward 90 days
vm.warp(block.timestamp + 91 days);
// Owner closes pot
pot.closePot();
// Expected result:
// - Manager: 800 / 10 = 80 tokens
// - Each claimant: (800 - 80) / 2 = 360 tokens
// - Total distributed: 80 + (2 * 360) = 800 tokens ✓
// ACTUAL result:
// - Manager: 80 tokens ✓
// - Each claimant: (800 - 80) / 10 = 72 tokens (BUG!)
// - Total distributed: 80 + (2 * 72) = 224 tokens
// - LOCKED FOREVER: 800 - 224 = 576 tokens (72% LOST!)
assertEq(token.balanceOf(players[0]), 100 + 72); // 172 instead of 460
assertEq(token.balanceOf(players[1]), 100 + 72); // 172 instead of 460
assertEq(token.balanceOf(address(pot)), 576); // STUCK FOREVER
}

Recommended Mitigation

- remove this code// 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.safeTransfer(msg.sender, managerCut);
+ uint256 remainingAfterCut = remainingRewards - managerCut;
+ // Handle case where no one claimed
+ if (claimants.length == 0) {
+ // No claimants, send all remaining to manager
+ i_token.safeTransfer(msg.sender, remainingAfterCut);
+ } else {
+ // Divide by actual claimants, not total players
- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
+ uint256 claimantCut = remainingAfterCut / claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
- _transferReward(claimants[i], claimantCut);
+ i_token.safeTransfer(claimants[i], claimantCut);
}
+ // Handle dust from integer division
+ uint256 distributed = claimantCut * claimants.length;
+ uint256 dust = remainingAfterCut - distributed;
+ if (dust > 0) {
+ i_token.safeTransfer(msg.sender, dust);
+ }
+ }
}
}
+ add this code
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!