MyCut

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

[H-3] Integer Division and Incorrect Denominator in `closePot()` Causes Permanent Fund Loss

[H-3] Integer Division and Wrong Denominator in closePot() Causes Permanent Fund Loss

Description

closePot() incorrectly calculates distributions, causing significant portions of remaining rewards to be permanently locked in the contract:

  1. Integer division precision loss: managerCut = remainingRewards / 10 rounds down small amounts

  2. Wrong denominator: Divides by i_players.length instead of claimants.length

  3. Funds never distributed: remainingRewards not updated after distribution

Risk

Likelihood: High

  • Occurs in every contest where not all players claim rewards

  • Automatic whenever remainingRewards > 0 at closure time

  • No special conditions required

Impact: High

  • Permanent fund loss: Tokens become inaccessible forever

  • 60-80% loss rate: Significant percentage of remaining rewards lost

  • Affects all parties: Both manager and claimants lose funds

  • No recovery: Stuck funds cannot be retrieved by anyone

Severity: High (H)

  • Direct, irreversible financial loss

  • Affects core contract functionality

  • No recovery mechanism exists

Proof of Concept

What This PoC Proves:

Step-by-step explanation:

Setup (Lines 1-25):

    3 players, each should get 15 wei

    Total pot: 45 wei

    Pot is fully funded

Claim Scenario (Lines 28-34):

    Only Player A claims (15 wei)

    Players B and C don't claim

    30 wei remains for distribution

Bug Trigger (Lines 37-42):

    Wait 90+ days (past claim period)

    Call closePot() - this is where bugs happen

Mathematical Proof (Lines 45-72):

    Bug 1: managerCut = 30 / 10 = 3 wei ✓ (correct but shows integer division)

    Bug 2: claimantCut = (30-3) / 3 = 9 wei ❌ (WRONG! Should be / 1 for 1 claimant)

    Bug 3: Only 1 claimant gets paid (9 wei), but calculation assumed 3 claimants

    Result: Total distributed = 3 + 9 = 12 wei

    Lost forever: 30 - 12 = 18 wei (60% loss!)

Verification (Lines 75-88):

    Check pot balance before/after

    Prove 18 wei stuck in contract

    Show funds can never be recovered
function test_PermanentFundLoss_PoC() public {
// Setup: 3 players, 45 wei total, only 1 claims
address[] memory players = new address[](3);
uint256[] memory rewards = new uint256[](3);
players[0] = address(0xA);
players[1] = address(0xB);
players[2] = address(0xC);
rewards[0] = rewards[1] = rewards[2] = 15; // wei
Pot pot = new Pot(players, rewards, weth, 45);
weth.transfer(address(pot), 45);
// Only player A claims
vm.prank(address(0xA));
pot.claimCut(); // Claims 15 wei
// Wait and close
vm.warp(block.timestamp + 91 days);
pot.closePot();
// Math: 30 wei remaining
// managerCut = 30 / 10 = 3 wei
// claimantCut = (30 - 3) / 3 = 9 wei (WRONG: divides by 3 players, not 1 claimant)
// Only 1 claimant gets 9 wei
// Total distributed: 3 + 9 = 12 wei
// LOST FOREVER: 18 wei (60%)
uint256 remaining = weth.balanceOf(address(pot));
assertEq(remaining, 18, "60% of funds permanently locked");
}

Recommended Mitigation

  1. Fixed-point math: (remaining * 1000) / 10000 avoids integer division rounding

  2. Correct denominator: Uses claimants.length not i_players.length

  3. State management: Sets remainingRewards = 0 and closed = true

  4. Minimal changes: Preserves existing architecture

function closePot() external onlyOwner {
require(block.timestamp - i_deployedAt >= 90 days, "Still open");
require(!closed, "Already closed");
closed = true;
uint256 remaining = remainingRewards;
if (remaining > 0 && claimants.length > 0) {
// Fixed-point for 10% (avoids integer division issues)
uint256 managerCut = (remaining * 1000) / 10000; // 10%
// Divide among ACTUAL claimants
uint256 claimantCut = (remaining - managerCut) / claimants.length;
i_token.transfer(owner(), managerCut);
for (uint256 i = 0; i < claimants.length; i++) {
i_token.transfer(claimants[i], claimantCut);
}
}
remainingRewards = 0;
}

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!