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

Pot::closePot divides the leftover pool by i_players.length instead of claimants.length, permanently locking unclaimed rewards

prasadreddy

Root + Impact

Description

  • Normal behavior: after 90 days, `closePot()` pays the manager 10% of the unclaimed pool and splits the remainder equally among the players who claimed in time.

  • Issue: the share is computed by dividing by the number of all players (`i_players.length`), but it is paid only to claimants (`claimants.length`). Whenever not everyone claimed, each claimant is underpaid and the difference stays in the Pot forever, because the Pot has no withdraw function.

// Root cause in the codebase with @> marks to highlight the relevant section
​
```solidity
function closePot() external onlyOwner {
...
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
​
// @> divides by ALL players, but only claimants are paid below
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}
```

Risk

Likelihood:

  • Triggered whenever at least one player has not claimed when `closePot()` runs, which is exactly the case the 90-day window exists for.

  • Every pot with a non-zero `remainingRewards` is affected.

Impact:

  • Claimants receive `claimants / players` of what they are owed, and the rest of the pool is locked in the Pot permanently.

  • Nothing in `Pot` or `ContestManager` can move the leftover tokens out.

Proof of Concept

10 players are each owed 100 (pool 1000) and only player 0 claims. After 90 days the manager closes the pot: manager cut = 90, claimant pool = 810. Player 0 should receive 810 / 1 claimant but receives 810 / 10 players = 81, so 729 tokens stay locked.
​
Save as `test/PoC.t.sol` and run `forge test --mt test_closePotStrandsFunds -vv`.
​
```solidity
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import {Test, console} from "lib/forge-std/src/Test.sol";
import {ERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
import {ContestManager} from "../src/ContestManager.sol";
import {Pot} from "../src/Pot.sol";
​
contract MockToken is ERC20 {
constructor() ERC20("Mock", "MCK") {}
function mint(address to, uint256 amt) external { _mint(to, amt); }
}
​
contract PoC is Test {
function test_closePotStrandsFunds() public {
address manager = makeAddr("manager");
address[] memory players = new address[](10);
uint256[] memory rewards = new uint256[](10);
for (uint256 i; i < 10; i++) {
players[i] = vm.addr(i + 1);
rewards[i] = 100;
}
​
// 1. Create and fund a 1000-token contest
vm.startPrank(manager);
ContestManager cm = new ContestManager();
MockToken token = new MockToken();
token.mint(manager, 1000);
token.approve(address(cm), 1000);
address pot = cm.createContest(players, rewards, IERC20(address(token)), 1000);
cm.fundContest(0);
vm.stopPrank();
​
// 2. Only player 0 claims
vm.prank(players[0]);
Pot(pot).claimCut();
​
// 3. After 90 days the manager closes the pot
vm.warp(block.timestamp + 90 days);
vm.prank(manager);
cm.closeContest(pot);
​
// 4. Claimant gets 100 + 81 = 181 (expected 910); 729 stuck in Pot (expected 0)
console.log("claimant:", token.balanceOf(players[0]));
console.log("stuck :", token.balanceOf(pot));
assertEq(token.balanceOf(players[0]), 181);
assertEq(token.balanceOf(pot), 729);
}
}
```
​

Recommended Mitigation

`closePot()` pays only `claimants`, so the pool must be divided by `claimants.length`. The guard avoids a divide-by-zero revert (which would make the pot impossible to close) when nobody claimed. With this fix the PoC scenario pays player 0 910 tokens and leaves the Pot at 0.
​
```diff
- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
- for (uint256 i = 0; i < claimants.length; i++) {
- _transferReward(claimants[i], claimantCut);
- }
+ if (claimants.length > 0) {
+ uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
+ for (uint256 i = 0; i < claimants.length; i++) {
+ _transferReward(claimants[i], claimantCut);
+ }
+ }
```
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!