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

Incorrect Division Denominator in closePot Causes Frozen Protocol Funds and Underpayout to Claimants

joshuapaul1

Root + Impact

Description

  • The contract intends to distribute the remaining uncollected reward pool equally among the subset of players who successfully interacted with the protocol and claimed their initial shares within the designated 90-day window.

  • The implementation erroneously divides the remaining rewards by the total number of original players (i_players.length) rather than the number of valid on-time claimants (claimants.length). This calculation deficit causes the contract to underpay the eligible claimants and leaves a significant portion of the remaining token balance permanently frozen inside the contract structure with no administrative mechanism for extraction.

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);
​
// @> Division uses the total original players length instead of on-time claimants length
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
// @> The loop only iterates through actual claimants, applying the diluted rate
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}
​

Risk

Likelihood: Medium

  • This issue manifests on contract expiration when the number of actual claimants is less than the total number of invited players.

  • The error triggers during periods of low protocol engagement when individual players miss the 90-day deadline due to inactivity or lost access to their private keys.

Impact: High

  • Legitimate on-time claimants are financially penalized and receive drastically fewer rewards than the protocol documentation guarantees.

  • Leftover ERC-20 tokens are permanently locked inside the non-upgradeable contract space without alternative drainage or rescue paths.Proof of Concept


Proof Of Concept

This test sets up a scenario with 3 total players, where only 1 player claims on time. It proves that instead of receiving the entire remainder as dictated by the business rules, the single on-time claimant is underpaid, and the remaining tokens are permanently locked.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import {Test, console} from "forge-std/Test.sol";
import {Pot} from "../src/Pot.sol"; // Adjust path to your contract
import {ERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
​
// Minimal ERC20 for funding the pot cleanly
contract MockToken is ERC20 {
constructor() ERC20("Test Token", "TT") {
_mint(msg.sender, 1000 ether);
}
}
​
contract PotMathPoCTest is Test {
Pot public pot;
MockToken public token;
​
address public owner = address(0x9999);
address public player1 = address(0x1111);
address public player2 = address(0x2222);
address public player3 = address(0x3333);
​
function setUp() public {
vm.startPrank(owner);
token = new MockToken();
​
address[] memory players = new address[](3);
players[0] = player1;
players[1] = player2;
players[2] = player3;
​
uint256[] memory rewards = new uint256[](3);
rewards[0] = 100 ether;
rewards[1] = 100 ether;
rewards[2] = 100 ether;
​
uint256 totalRewards = 300 ether;
​
// Deploy the Pot contract
pot = new Pot(players, rewards, token, totalRewards);
​
// Explicitly fund the contract with the correct rewards balance
token.transfer(address(pot), totalRewards);
vm.stopPrank();
}
​
function test_poc_division_bug_locks_funds() public {
// --- 1. Initial State Check ---
// Contract has 300 ether. 3 total players.
assertEq(token.balanceOf(address(pot)), 300 ether);
​
// --- 2. Action: Only Player 1 claims on time ---
vm.prank(player1);
pot.claimCut();
​
// Player 1 successfully took their base 100 ether
assertEq(token.balanceOf(player1), 100 ether);
// Remaining rewards inside the contract tracking state is now 200 ether
uint256 remainingRewardsBeforeClose = pot.getRemainingRewards();
assertEq(remainingRewardsBeforeClose, 200 ether);
​
// --- 3. Action: 90 days pass and Manager closes the Pot ---
skip(91 days);
​
uint256 managerBalanceBefore = token.balanceOf(owner);
vm.prank(owner);
pot.closePot();
​
// --- 4. Math Breakdown and Bug Verification ---
// Expected Manager Cut: 10% of 200 ether = 20 ether
uint256 expectedManagerCut = 20 ether;
assertEq(token.balanceOf(owner) - managerBalanceBefore, expectedManagerCut);
​
// Expected Leftover for Claimants: 200 ether - 20 ether = 180 ether
// Since ONLY Player 1 claimed, Player 1 should receive ALL 180 ether.
// BUT the contract divides by i_players.length (3) instead of claimants.length (1):
// 180 ether / 3 = 60 ether per claimant.
// Player 1 gets 60 ether extra instead of 180 ether
uint256 expectedPlayer1Extra = 60 ether;
assertEq(token.balanceOf(player1), 100 ether + expectedPlayer1Extra);
console.log("Player 1 received extra payout of:", expectedPlayer1Extra / 1e18, "tokens");
​
// --- 5. Proof of Frozen Funds ---
// Total Spent = 100 (P1 Claim) + 20 (Manager) + 60 (P1 Extra) = 180 ether
// Leftover Locked = 300 - 180 = 120 ether
uint256 lockedBalance = token.balanceOf(address(pot));
console.log("Tokens permanently frozen in contract:", lockedBalance / 1e18, "tokens");
assertEq(lockedBalance, 120 ether);
assertTrue(lockedBalance > 0, "Funds should have been drained completely but tokens are locked!");
}
}

Run this command in your project workspace terminal to see the test logs confirm the trapped funds:

forge test --match-test test_poc_division_bug_locks_funds -vv

Recommended Mitigation

Update the calculation inside closePot() to use claimants.length instead of i_players.length to distribute the remainder strictly among active participants. A safety check must also be added to handle cases where zero users claim, preventing a runtime division-by-zero error. [1]

Additionally, clear the remainingRewards state variable at the end of the operation to ensure the contract cannot be closed multiple times.

// @> Define a custom error for the empty claimant edge-case
error Pot__NoClaimants();
​
function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
// @> Prevention: Halt execution if nobody claimed within 90 days
if (claimants.length == 0) {
revert Pot__NoClaimants();
}
​
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
​
// @> Remediation: Divide remaining rewards strictly by valid on-time claimants
uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
​
// @> Prevention: Reset state tracking variable to block subsequent manipulation
remainingRewards = 0;
}
}


Updates

Lead Judging Commences

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