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

## Incorrect distribution of remaining rewards in `closePot()` – Wrong divisor for claimant cuts leads to unfair distribution and permanently stuck funds

hark017

[H-02] Incorrect distribution of remaining rewards in closePot() – Wrong divisor for claimant cuts leads to unfair distribution and permanently stuck funds

Severity: High Location: Pot.sol, closePot() function (lines relevant to the calculation and loop)

Description

In the closePot() function, which can only be called by the owner after 90 days, any unclaimed rewards (remainingRewards) are partially distributed as follows:

  • A manager cut is taken:remainingRewards / managerCutPercent(with managerCutPercent = 10, this effectively gives the manager 10% of the remaining rewards, as remaining / 10 = 10%).

  • The bonus pool is then (remainingRewards - managerCut), intended to be shared among the claimants (players who have already claimed their initial rewards).

However, the per-claimant bonus (claimantCut) is calculated by dividing the bonus pool by i\_players.length (the total number of original players eligible at deployment), rather than by claimants.length (the actual number of players who have claimed).

The distribution loop then iterates only over claimants.length, sending claimantCut to each claimant.

This mismatch creates several problems:

  • If fewer than all players have claimed (claimants.length < i\_players.length), each claimant receives a smaller bonus than intended (bonus pool divided by a larger number), and the total bonus distributed is less than the available bonus pool. The undistributed portion (bonus pool) - (claimantCut \* claimants.length) remains permanently stuck in the contract, as there is no mechanism to recover it.

  • If no players have claimed (claimants.length == 0), the loop does nothing, so after the manager cut (10%), the remaining 90% of unclaimed rewards is permanently locked in the contract.

  • Only if all players have claimed (claimants.length == i\_players.length) does the distribution work approximately as intended (modulo integer division losses, which could still leave tiny amounts stuck due to rounding down).

This is noted in the code comment: "//@audit - should check the length of the claimant list's length to distribute the remaining rewards".

Additionally, the manager cut calculation is hardcoded to divide by 10, which works for the 10% constant but could be made more flexible and clear using percentage math (remainingRewards * managerCutPercent) / 100.

The overall logic assumes that unclaimed rewards should be bonused only to claimants, but the divisor uses the wrong array length, leading to under-distribution.

Impact

  • Permanently Stuck Funds: In cases where not all players claim (realistic scenario, e.g., inactive players), a portion of the remaining rewards (proportional to the unclaimed ratio) becomes irrecoverable, leading to loss of tokens for the protocol and users.

  • Unfair Distribution: Claimants receive less than their fair share of the bonus, disincentivizing participation or leading to disputes.

  • Edge Case Lockup: If zero claims occur (e.g., abandoned pot), 90% of the total rewards (after manager's 10%) are stuck forever, defeating the purpose of the pot.

  • Economic Loss: Affects trust in the system; managers might deploy pots knowing funds can lock up, or honest errors lead to unrecoverable ERC20 tokens.

  • This is exploitable indirectly if a manager deploys with many fake players (increasing i_players.length) and ensures few real claims, maximizing stuck funds after taking the manager cut.

The severity is high due to the potential for significant, permanent fund loss without requiring complex exploits—just normal usage with partial claims.

Proof of Concept

The following Foundry test demonstrates the issue. It uses a setup with 3 players, funds the pot correctly, has only 1 or 0 players claim, then closes the pot after 90 days, and asserts that funds are stuck.

pragma solidity ^0.8.20;
​
import {Test} from "../lib/forge-std/src/Test.sol";
import {Pot} from "../src/Pot.sol";
import {MockERC20} from "./ERC20Mock.sol";
import {IERC20} from "../lib/openzeppelin-contracts/contracts/interfaces/IERC20.sol";
​
// test/PotCloseTest.t.sol
pragma solidity ^0.8.20;
​
contract PotCloseTest is Test {
Pot pot;
MockERC20 token;
address[] players;
uint256[] rewards;
address owner = address(this); // Test contract as owner
address player1 = address(0x1);
address player2 = address(0x2);
address player3 = address(0x3);
​
function setUp() public {
token = new MockERC20("erc" , "ERC" , owner , 0 ether );
token.mint(address(this), 300 ether);
​
players = new address[](3);
players[0] = player1;
players[1] = player2;
players[2] = player3;
​
rewards = new uint256[](3);
rewards[0] = 100 ether;
rewards[1] = 100 ether;
rewards[2] = 100 ether; // Sum = 300 ether
}
​
function test_ClosePotPartialClaimsStuckFunds() public {
pot = new Pot(players, rewards, token, 300 ether);
token.transfer(address(pot), 300 ether); // Fund exactly
​
// Only 1 player claims
vm.prank(player1);
pot.claimCut(); // Claims 100 ether, remaining = 200 ether
​
// Warp to after 90 days
vm.warp(block.timestamp + 91 days);
​
// Close the pot
pot.closePot();
​
// Calculations:
// remainingRewards = 200 ether
// managerCut = 200 / 10 = 20 ether
// bonusPool = 200 - 20 = 180 ether
// claimantCut = 180 / 3 (players.length) = 60 ether
// Distributed: 60 ether to the 1 claimant (loop over claimants.length=1)
// Total distributed in close: 20 (manager) + 60 = 80 ether
// Stuck: 200 - 80 = 120 ether (should be 0 if divided by claimants.length)
​
assertEq(token.balanceOf(address(pot)), 120 ether); // Proves funds stuck
assertEq(token.balanceOf(player1), 100 ether + 60 ether); // Initial + bonus
assertEq(token.balanceOf(owner), 20 ether); // Manager cut
}
​
function test_ClosePotZeroClaimsFullyStuck() public {
pot = new Pot(players, rewards, IERC20(token), 300 ether);
token.transfer(address(pot), 300 ether);
​
// No claims: remaining = 300 ether, claimants.length=0
​
vm.warp(block.timestamp + 91 days);
pot.closePot();
​
// managerCut = 300 / 10 = 30 ether
// bonusPool = 270 ether
// claimantCut = 270 / 3 = 90 ether
// Distributed bonus: 0 (loop does nothing)
// Stuck: 270 ether (90% of remaining)
​
assertEq(token.balanceOf(address(pot)), 270 ether); // Proves 90% stuck
assertEq(token.balanceOf(owner), 30 ether);
}
}

To run: forge test --match-test test_ClosePotPartialClaimsStuckFunds (and similarly for the other). It will pass if the bug exists, showing stuck funds via assertions.

Note: In real deployment, add sum validation from [H-01] to avoid compounding issues, but this POC assumes correct sum for isolation.

Recommended mitigation

  • Change the divisor in claimantCut calculation to claimants.length instead of i_players.length to fairly share the bonus among actual claimants.

  • Handle the zero-claimants case explicitly: e.g., if claimants.length == 0, transfer the entire bonus pool to the manager/owner, or revert, depending on desired policy. For example:

solidity

uint256 managerCut = (remainingRewards * managerCutPercent) / 100; // Clearer percentage calc
i_token.transfer(msg.sender, managerCut);
​
uint256 bonusPool = remainingRewards - managerCut;
if (claimants.length == 0) {
i_token.transfer(msg.sender, bonusPool); // Or revert Pot__NoClaimants();
} else {
uint256 claimantCut = bonusPool / claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}

  • This ensures no funds are stuck and distribution is fair.

  • Consider adding events for close actions to improve transparency.

  • Test for integer division losses (e.g., if bonusPool % claimants.length != 0, tiny amounts still stuck—could add to manager or last claimant).

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!