MyCut

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

H-01: Incorrect Distribution Calculation in closePot() - Divides by Total Players Instead of Claimants

Root + Impact

Root Cause

In Pot.sol line 73, the closePot() function incorrectly calculates each claimant's share by dividing remaining rewards (after manager cut) by i_players.length (total players) instead of claimants.length (actual claimants):

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
contract Pot is Ownable(msg.sender) {
// ... state variables ...
address[] private i_players; // All registered players
address[] private claimants; // Players who actually claimed
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);
// @> ROOT CAUSE: Divides by i_players.length instead of claimants.length
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}
}

Impact

  • Systematic underpayment to claimants proportional to (claimants.length / i_players.length)

  • If only 1 of 100 players claims, that claimant receives 1% of their rightful share

  • Manager takes correct 10% cut, but remaining 90% distributed incorrectly

  • Claimants cannot receive their fair share of unclaimed rewards


Description

The closePot() function incorrectly calculates each claimant's share of remaining rewards by dividing by the total number of players (i_players.length) instead of the actual number of claimants (claimants.length).

// Line 73 in Pot.sol - BUGGY
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length; // Should be claimants.length
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}

Risk

Likelihood:

Mathematical Analysis

Scenario Players Claimants Remaining Manager Cut (10%) Buggy Share Correct Share Loss %
1 of 3 claims 3 1 200 20 60 180 66.7%
2 of 10 claims 10 2 900 90 81 405 80%
5 of 100 claims 100 5 10000 1000 90 1800 95%

Formula: Loss = 1 - (claimants.length / i_players.length)


Impact:

Dimension Assessment
Fund Loss Direct misallocation - claimants underpaid
Attack Vector Malicious owner could create pots with many dummy players to reduce payouts
Fairness Severe - only 1/N of rightful share where N = total players
Trust Protocol appears to steal from claimants

Proof of Concept

Test: test_POC_H01_IncorrectDistributionCalculation()

function test_POC_H01_IncorrectDistributionCalculation() public {
address[] memory players = new address[](3);
players[0] = player1;
players[1] = player2;
players[2] = player3;
uint256[] memory rewards = new uint256[](3);
rewards[0] = 100;
rewards[1] = 100;
rewards[2] = 100;
vm.startPrank(user);
potAddr = conMan.createContest(players, rewards, IERC20(weth), 300);
pot = Pot(potAddr);
conMan.fundContest(0); // Fund with 300 tokens
vm.stopPrank();
// Only player1 claims (100 tokens)
vm.startPrank(player1);
pot.claimCut();
vm.stopPrank();
// 200 remaining, manager takes 10% = 20, 180 to distribute
// BUG: divides by 3 (i_players.length) instead of 1 (claimants.length)
// So each claimant gets 60 instead of 180
vm.warp(91 days);
uint256 balBefore = weth.balanceOf(player1);
vm.startPrank(user);
conMan.closeContest(potAddr);
vm.stopPrank();
uint256 balAfter = weth.balanceOf(player1);
uint256 received = balAfter - balBefore;
console.log("Player1 received:", received);
console.log("Expected (correct): 180");
console.log("Actual (buggy): 60");
// This demonstrates the bug - player1 gets 60 instead of 180
assertEq(received, 60); // Bug confirmed
}

Result: ✅ PASSED

[PASS] test_POC_H01_IncorrectDistributionCalculation() (gas: 1166807)
Logs:
Expected (correct): 180
Actual (buggy): 60

Reproduction Steps

  1. Create pot with N players, fund with tokens

  2. Have only M players claim (M < N)

  3. Wait 90 days

  4. Call closePot()

  5. Claimants receive (remaining - managerCut) / N instead of / M


Recommended Mitigation

// Line 73 - FIXED
uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;

Additional Safety: Add zero check

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

Updates

Lead Judging Commences

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