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

Pot::closePot divides the claimant bonus by i_players.length instead of claimants.length, so in-time claimants are underpaid and the non-claimers' share is stranded in the Pot

friedsocks233

Bonus divided by all players instead of by claimants + in-time claimants underpaid and the remainder stranded

Description

  • Each player in a Pot claims their assigned reward with claimCut(). After the 90-day window the owner closes the pot through ContestManager::closeContest. The README says that "the manager takes a cut of the remaining pool and the remainder is distributed equally to those who claimed in time". So after the 10% manager cut, the whole unclaimed remainder should be split among the players who claimed.

  • Pot::closePot divides the post-cut remainder by i_players.length, which counts every player, including those who never claimed. The loop then pays only the claimants.length addresses. remainingRewards > 0 at close only when at least one player did not claim, so every real distribution pays out C/N of the bonus and leaves (N - C) * claimantCut in the Pot (C = claimants, N = players). With zero claimants the loop does not run at all and 90% of the pool stays in the Pot. Neither Pot nor ContestManager has a sweep, withdraw or rescue function, so this leftover has no intended exit.

// src/Pot.sol
49: function closePot() external onlyOwner {
50: if (block.timestamp - i_deployedAt < 90 days) {
51: revert Pot__StillOpenForClaim();
52: }
53: if (remainingRewards > 0) {
54: uint256 managerCut = remainingRewards / managerCutPercent;
55: i_token.transfer(msg.sender, managerCut);
56:
@> 57: uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
@> 58: for (uint256 i = 0; i < claimants.length; i++) {
59: _transferReward(claimants[i], claimantCut);
60: }
61: }
62: }

Risk

Likelihood:

  • This occurs on every pot where at least one player does not claim before the close. That is the exact scenario the redistribution step exists for. No attacker and no admin error is needed: the trusted owner calling closeContest after 90 days, as designed, triggers it.

  • The zero-claimant variant occurs whenever no player claims during the window. The payout loop then runs zero times.

Impact:

  • In-time claimants receive only C/N of the bonus the README promises. With two players at 500 each and only p1 claiming, p1 receives a bonus of 225 instead of 450. With four players at 100 each and one claimer, p1 receives 67 instead of 270.

  • The unpaid share stays in the Pot with no intended recovery path. In the two-player case the 225 is frozen for good: p2's late claimCut (needs 500) and a second closeContest (needs 275) both revert with ERC20InsufficientBalance. With zero claimants, 900 of a 1000 pool stays in the Pot. Depending on balances, late non-claimers can instead take part of the leftover through the separate missing-deadline issue (shown in the four-player test). The in-time claimant is still not paid what they are owed.

Proof of Concept

The test deploys ContestManager and the repo's test/ERC20Mock.sol directly. It goes through createContest, fundContest, claimCut and closeContest, so the Pot is owned by ContestManager as in production. All four tests pass:

  • test_Baseline_AllClaim_NothingStranded (control): both players claim, the close moves nothing, and the Pot ends empty.

  • test_MC01_TwoPlayers_ClaimantUnderpaid_And_RemainderLocked: the manager cut is 50. p1's bonus is 225 instead of 450, and 225 is stranded. p2's claim reverts with ERC20InsufficientBalance(pot, 225, 500). A second close reverts with ERC20InsufficientBalance(pot, 175, 225). The Pot still holds 225 at the end.

  • test_MC01_FourPlayers_ClaimantUnderpaid: the bonus is 67 instead of 270, and 203 is left in the Pot. Two late non-claimers then take 100 each, and p1 never receives the rest of what it is owed.

  • test_MC01_ZeroClaimants_NinetyPercentLeftInPot: the manager cut is 100 and 900 stays in the Pot.

Save the file as test/PoC_MC-01.t.sol and run from the repository root:

FOUNDRY_FFI=false forge test --match-path test/PoC_MC-01.t.sol -vv
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import {Test, console} from "lib/forge-std/src/Test.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {IERC20Errors} from "lib/openzeppelin-contracts/contracts/interfaces/draft-IERC6093.sol";
import {ContestManager} from "../src/ContestManager.sol";
import {Pot} from "../src/Pot.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
​
/// MC-01: closePot divides the claimant bonus by i_players.length instead of claimants.length.
contract PoC_MC01 is Test {
address admin = makeAddr("admin");
address p1 = makeAddr("p1");
address p2 = makeAddr("p2");
address p3 = makeAddr("p3");
address p4 = makeAddr("p4");
​
ContestManager conMan;
ERC20Mock token;
​
function setUp() public {
vm.startPrank(admin);
conMan = new ContestManager();
token = new ERC20Mock("TKN", "TKN", admin, 0);
token.mint(admin, 1_000_000);
token.approve(address(conMan), type(uint256).max);
vm.stopPrank();
}
​
function _createAndFund(address[] memory players, uint256[] memory rewards, uint256 total)
internal
returns (Pot pot)
{
vm.startPrank(admin);
address potAddr = conMan.createContest(players, rewards, IERC20(address(token)), total);
uint256 idx = conMan.getContests().length - 1;
conMan.fundContest(idx);
vm.stopPrank();
pot = Pot(potAddr);
}
​
function _twoPlayers() internal view returns (address[] memory players, uint256[] memory rewards) {
players = new address[](2);
players[0] = p1;
players[1] = p2;
rewards = new uint256[](2);
rewards[0] = 500;
rewards[1] = 500;
}
​
// Baseline: when everyone claims, nothing is left and closePot is a no-op.
function test_Baseline_AllClaim_NothingStranded() public {
(address[] memory players, uint256[] memory rewards) = _twoPlayers();
Pot pot = _createAndFund(players, rewards, 1000);
​
vm.prank(p1);
pot.claimCut();
vm.prank(p2);
pot.claimCut();
​
vm.warp(block.timestamp + 90 days);
vm.prank(admin);
conMan.closeContest(address(pot));
​
assertEq(token.balanceOf(p1), 500);
assertEq(token.balanceOf(p2), 500);
assertEq(token.balanceOf(address(pot)), 0, "baseline: pot fully drained");
}
​
// Headline: [500,500], only p1 claims. README says the 450 remainder (after the 10% manager cut)
// goes to "those who claimed in time" -> p1 should get 450. Code pays 450/2 = 225 and strands 225.
function test_MC01_TwoPlayers_ClaimantUnderpaid_And_RemainderLocked() public {
(address[] memory players, uint256[] memory rewards) = _twoPlayers();
Pot pot = _createAndFund(players, rewards, 1000);
assertEq(token.balanceOf(address(pot)), 1000);
​
vm.prank(p1);
pot.claimCut();
assertEq(token.balanceOf(p1), 500);
assertEq(pot.getRemainingRewards(), 500);
​
vm.warp(block.timestamp + 90 days);
vm.prank(admin);
conMan.closeContest(address(pot));
​
uint256 managerCut = 500 / 10; // 50
uint256 intendedBonus = (500 - managerCut) / 1; // 450, divided by claimants (1)
uint256 actualBonus = token.balanceOf(p1) - 500;
​
console.log("intended bonus for p1 :", intendedBonus);
console.log("actual bonus for p1 :", actualBonus);
console.log("manager cut received :", token.balanceOf(address(conMan)));
console.log("stranded in Pot :", token.balanceOf(address(pot)));
​
assertEq(token.balanceOf(address(conMan)), 50, "manager cut");
assertEq(actualBonus, 225, "p1 paid only half the bonus");
assertLt(actualBonus, intendedBonus, "claimant underpaid");
assertEq(token.balanceOf(address(pot)), 225, "225 stranded in Pot");
​
// No path moves the 225: p2's late claim needs 500 -> reverts.
vm.prank(p2);
vm.expectRevert(
abi.encodeWithSelector(IERC20Errors.ERC20InsufficientBalance.selector, address(pot), 225, 500)
);
pot.claimCut();
​
// A second close needs 50 + 225 = 275 -> manager transfer succeeds, claimant transfer reverts,
// whole call reverts.
vm.prank(admin);
vm.expectRevert(
abi.encodeWithSelector(IERC20Errors.ERC20InsufficientBalance.selector, address(pot), 175, 225)
);
conMan.closeContest(address(pot));
​
assertEq(token.balanceOf(address(pot)), 225, "225 still locked after all attempts");
}
​
// 4 players x 100, only p1 claims. Intended bonus = 270; code pays (300-30)/4 = 67, 203 left in Pot.
function test_MC01_FourPlayers_ClaimantUnderpaid() public {
address[] memory players = new address[](4);
players[0] = p1;
players[1] = p2;
players[2] = p3;
players[3] = p4;
uint256[] memory rewards = new uint256[](4);
for (uint256 i; i < 4; i++) rewards[i] = 100;
Pot pot = _createAndFund(players, rewards, 400);
​
vm.prank(p1);
pot.claimCut();
​
vm.warp(block.timestamp + 90 days);
vm.prank(admin);
conMan.closeContest(address(pot));
​
uint256 actualBonus = token.balanceOf(p1) - 100;
console.log("4x100 intended bonus :", uint256(270));
console.log("4x100 actual bonus :", actualBonus);
console.log("4x100 left in Pot :", token.balanceOf(address(pot)));
​
assertEq(actualBonus, 67, "p1 gets 1/4 of the 270 bonus");
assertEq(token.balanceOf(address(conMan)), 30);
assertEq(token.balanceOf(address(pot)), 203, "203 of 400 stays in Pot");
​
// Nuance (refuter correction): the leftover is not always frozen. claimCut has no deadline,
// so non-claimers can still take their reward from it after close.
vm.prank(p2);
pot.claimCut();
vm.prank(p3);
pot.claimCut();
assertEq(token.balanceOf(p2), 100);
assertEq(token.balanceOf(p3), 100);
assertEq(token.balanceOf(address(pot)), 3, "late claimers drained the stranded share");
// p1 (the in-time claimant) still never receives the 203 it was owed beyond 67.
assertEq(token.balanceOf(p1), 167);
}
​
// Zero claimants: closePot pays the 100 manager cut and leaves 900 in the Pot.
function test_MC01_ZeroClaimants_NinetyPercentLeftInPot() public {
(address[] memory players, uint256[] memory rewards) = _twoPlayers();
Pot pot = _createAndFund(players, rewards, 1000);
​
vm.warp(block.timestamp + 90 days);
vm.prank(admin);
conMan.closeContest(address(pot));
​
console.log("zero-claimant manager cut :", token.balanceOf(address(conMan)));
console.log("zero-claimant left in Pot :", token.balanceOf(address(pot)));
​
assertEq(token.balanceOf(address(conMan)), 100);
assertEq(token.balanceOf(address(pot)), 900, "90% of the pool stays in the Pot");
}
}

Recommended Mitigation

Divide by the number of claimants. When nobody claimed, and for rounding dust, pay the rest to the manager instead of leaving it in the Pot. Combine this with the fix for the separate locked-manager-cut issue so that this amount actually reaches the admin, and with the claim-deadline fix so that claimants holds only in-time claimers.

--- a/src/Pot.sol
+++ b/src/Pot.sol
@@ -52,12 +52,17 @@
}
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
- i_token.transfer(msg.sender, managerCut);
-
- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
- for (uint256 i = 0; i < claimants.length; i++) {
- _transferReward(claimants[i], claimantCut);
+ uint256 claimantPool = remainingRewards - managerCut;
+ uint256 paidToClaimants;
+ if (claimants.length > 0) {
+ uint256 claimantCut = claimantPool / claimants.length;
+ for (uint256 i = 0; i < claimants.length; i++) {
+ _transferReward(claimants[i], claimantCut);
+ }
+ paidToClaimants = claimantCut * claimants.length;
}
+ // no claimant, or rounding dust: pay it to the manager instead of stranding it in the Pot
+ i_token.transfer(msg.sender, managerCut + claimantPool - paidToClaimants);
}
}
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!