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

Stucked Funds

daspra0tik

Root + Impact

Incorrect math division and missing state updates permanently trap leftover ERC20 tokens inside the contract.


Description

  • When owner closes the pot the remaining reward should be distributed to the claimants. But it did distributed the claimants but when calculating the claimantCut, the contract divides the leftover pool by the total number of initial players (i_players.length) rather than the active ones (claimants.length). The slices meant for inactive players are never distributed. Furthermore, the contract forgets to update the remainingRewards state variable, leaving the unassigned tokens permanently stuck with no rescue mechanism.

function claimCut() public {
address player = msg.sender;
uint256 reward = playersToRewards[player];
if (reward <= 0) {
revert Pot__RewardNotFound();
}
​
playersToRewards[player] = 0;
remainingRewards -= reward;
claimants.push(player);
_transferReward(player, reward);
}
​
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;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

Risk

Likelihood: High

  • Reason 1: Occurs every single time closePot() is successfully executed.

  • Reason 2: Guaranteed to happen whenever at least one original player fails to claim their cut during the active window.

Impact: High

  • Impact 1: Permanent loss/freezing of ERC20 assets inside the Pot contract.

  • Impact 2: Active claimants receive a significantly smaller final payout than mathematically intended.

Proof of Concept

function test_StuckRewards() public createMultipleAddress {
//Arrange
​
/*
Initital Phase
*/
uint256 initial_balanceOfSpider = merc20.balanceOf(spider);
uint256 initial_balanceOfAddress1 = merc20.balanceOf(address(1));
uint256 initial_balanceOfAddress2 = merc20.balanceOf(address(2));
​
uint256 Initial_balanceOfContest = merc20.balanceOf(
address(modifier_contest)
);
​
uint256 balanceOf_cmanager = merc20.balanceOf(address(cmanager));
​
//Act
​
/*
after Fund Phase
*/
cmanager.fundContest(1);
​
uint256 After_Fund_Balance_Of_Contest = merc20.balanceOf(
address(modifier_contest)
);
​
/*
after claim cut of some user
*/
​
vm.startPrank(spider);
Pot(modifier_contest).claimCut();
vm.stopPrank();
​
vm.startPrank(address(1));
Pot(modifier_contest).claimCut();
vm.stopPrank();
​
vm.startPrank(address(2));
Pot(modifier_contest).claimCut();
vm.stopPrank();
​
uint256 balance_of_spider_after_claim_cut = merc20.balanceOf(spider);
uint256 balance_of_address1_after_claim_cut = merc20.balanceOf(
address(1)
);
uint256 balance_of_address2_after_claim_cut = merc20.balanceOf(
address(2)
);
​
/*
phase after 91 days
*/
​
vm.warp(block.timestamp + 91 days);
vm.startPrank(default_Foundry);
cmanager.closeContest(address(modifier_contest));
vm.stopPrank();
​
uint256 balance_Of_cmanager_after_closing_the_pot = merc20.balanceOf(
address(cmanager)
);
​
uint256 balanc_of_contest_after_close_the_pot = merc20.balanceOf(
address(modifier_contest)
);
​
uint256 balance_of_spider_after_closing_the_pot = merc20.balanceOf(
spider
);
uint256 balance_of_address1_after_closing_the_pot = merc20.balanceOf(
address(1)
);
uint256 balance_of_address2_after_closing_the_pot = merc20.balanceOf(
address(2)
);
​
vm.startPrank(address(3));
Pot(modifier_contest).claimCut();
vm.stopPrank();
​
vm.startPrank(address(4));
Pot(modifier_contest).claimCut();
vm.stopPrank();
​
uint256 balance_of_other_user = merc20.balanceOf(address(3));
uint256 balance_of_other_user2 = merc20.balanceOf(address(4));
​
uint256 After_PotRemainingRewards = cmanager.getContestRemainingRewards(
address(modifier_contest)
);
​
uint256 state_check_Pot_RemainingBalance = merc20.balanceOf(
address(modifier_contest)
);
​
//Assert
​
//Initial Asserts
assertEq(initial_balanceOfSpider, 0);
assertEq(initial_balanceOfAddress1, 0);
assertEq(initial_balanceOfAddress2, 0);
​
assertEq(Initial_balanceOfContest, 0);
​
assertEq(balanceOf_cmanager, 0);
​
//After fund
​
assertEq(After_Fund_Balance_Of_Contest, 21e18);
​
//After some user claim cut
assertEq(balance_of_spider_after_claim_cut, 1e18);
assertEq(balance_of_address1_after_claim_cut, 1e18);
assertEq(balance_of_address2_after_claim_cut, 1e18);
​
//After close pot the claimant get some extra funds
assertGt(balance_of_spider_after_closing_the_pot, 1e18);
assertGt(balance_of_address1_after_closing_the_pot, 1e18);
assertGt(balance_of_address2_after_closing_the_pot, 1e18);
​
assertGt(balance_Of_cmanager_after_closing_the_pot, balanceOf_cmanager);
​
assertLt(
balanc_of_contest_after_close_the_pot,
After_Fund_Balance_Of_Contest
);
​
//after other user claim cut
assertEq(balance_of_other_user, 1e18);
assertEq(balance_of_other_user2, 1e18);
​
//state_check
assertGt(After_PotRemainingRewards, state_check_Pot_RemainingBalance);
​
//Stuck_Bug
​
assertGt(merc20.balanceOf(address(modifier_contest)), 0, "stuck");
}

Recommended Mitigation

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 leftoverFunds = remainingRewards - managerCut;
​
// FIX 1 & 2: Prevent Division by Zero and check if anyone actually claimed
if (claimants.length > 0) {
// FIX 3: Divide evenly among ACTIVE claimants only
uint256 claimantCut = leftoverFunds / claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
} else {
// SAFETY NET: If nobody claimed, send the leftovers to the manager
// so the 90% doesn't get permanently trapped inside the contract.
i_token.transfer(msg.sender, leftoverFunds);
}
​
// FIX 4: Explicitly update the state to prevent accounting desync
remainingRewards = 0;
}
}
Updates

Lead Judging Commences

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