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

Root cause: `closePot` splits the leftover pool by `i_players.length` instead of the number of claimants. Impact: everyone who claimed in time is underpaid and the shortfall is stranded in the Pot forever

hati

Root cause: closePot splits the leftover pool by i_players.length instead of the number of claimants. Impact: everyone who claimed in time is underpaid and the shortfall is stranded in the Pot forever

Description

The README describes the end of a contest in one sentence: after the 90 day window the manager takes a cut and "the remainder is distributed equally to those who claimed in time". The leftover belongs to the claimants and only to them, since players who never showed up forfeited their share.

closePot computes the manager cut correctly, then divides what is left by the wrong number. It uses i_players.length, everyone ever registered, rather than claimants.length, the list the loop right underneath actually pays out to. Those are only equal when every player claimed in time, which is exactly the case the leftover branch does not need to handle. Otherwise the divisor is too big, each claimant gets a fraction of what the spec promises, and the difference stays in the Pot with no function able to move it out.

// src/Pot.sol
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:

  • It triggers on every close where at least one registered player did not claim, which is the normal outcome of an airdrop style distribution and the only situation the leftover branch exists for.

  • Nothing special is required. The manager calls closeContest exactly as intended and the loss happens inside that call.

Impact:

  • Claimants receive leftover / players instead of leftover / claimants. With ten registered players and one claimant he gets a tenth of what he is owed, and with small rewards the division truncates to zero so he is paid nothing at all.

  • The shortfall is permanently unreachable. Pot has no sweep or rescue function, claimCut reverts once a mapping entry is zero, and calling closePot again reverts because the pot can no longer cover the recomputed payouts.

Proof of Concept

Two players are registered for 500 each, funded with 1000. Player one claims in time, player two never does. There is no attacker, the manager is honest and the loss comes from the protocol itself. After 91 days he closes. Remaining is 500, the cut is 50, so 450 is owed to the single claimant. He gets 225. Run forge test --match-test test_R1 -vv.

function test_R1_wrongDivisor_underpaysClaimant_andStrandsTokens() public {
Pot pot = _make(players, rewards, 1000); // p1 and p2, 500 each
​
vm.prank(p1);
pot.claimCut(); // only p1 claims in time
uint256 balBefore = weth.balanceOf(p1); // 500
​
assertEq(pot.getRemainingRewards(), 500); // p2's unclaimed share
​
vm.warp(block.timestamp + 91 days); // window expires
vm.prank(manager);
conMan.closeContest(address(pot));
​
uint256 paidOut = weth.balanceOf(p1) - balBefore;
assertEq(paidOut, 225); // spec says 450, code pays 225
​
assertEq(weth.balanceOf(address(pot)), 225); // the rest never leaves
}

Output:

p1 should have received 450, actually received 225
tokens stranded in Pot forever: 225

A fuzz run over 2000 random splits confirms the payout is always owed / playerCount. With 1000 recipients and 100 claiming in time, each is paid ten times less than the spec allows and 729 of the 810 leftover tokens stay stuck.

Worth noting: the sponsor's testUnclaimedRewardDistribution stays green after the fix, since it only asserts the balance went up, which holds either way. Only a differential run on identical inputs separates the two versions.

Recommended Mitigation

Divide by the list that is actually being paid, and guard the empty case so a contest with no claimants does not revert on a division by zero:

if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
uint256 leftover = remainingRewards - managerCut;
remainingRewards = 0; // settle before paying out
​
i_token.transfer(owner(), managerCut);
​
if (claimants.length > 0) {
uint256 claimantCut = leftover / claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

With claimants.length the claimant above receives the full 450 and the pot ends at zero, as the README promises.

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!