MyCut

Description

After the 90-day claim window, Pot.closePot() is supposed to take a 10% manager cut of the unclaimed remainder and split the rest equally among players who claimed on time. The README states this explicitly:

authorized claimants 90 days to claim before the manager takes a cut of the remaining pool and the remainder is distributed equally to those who claimed in time

The implementation does the opposite of that last clause. It computes each claimant's share by dividing the residual by the full player list, then iterates only the claimants array. Tokens allocated to non-claimants are never transferred, never written back, and never rescueable. They sit in the Pot forever.

This is not dust. For a contest with N players and C timely claimants (C < N), the locked amount is (N - C) * claimantCut — a non-dust multiple of the per-claimant share. If nobody claims, ~90% of the pot is stranded after the manager cut. No attacker is required: any mapped player who skips claimCut, plus the owner invoking the designed closeContest after 90 days, is enough.

Deep Dive

The root cause is a single divisor mismatch in src/Pot.sol.

Line 57 divides by i_players.length. Lines 58–59 pay only claimants. Those two sets are not the same after a partial claim.

Supporting state that makes the lock permanent:

• claimCut (src/Pot.sol:37-46) zeros the caller in playersToRewards, subtracts their original reward from remainingRewards, pushes them onto claimants, and transfers their original cut. It does not, and cannot, redistribute anyone else's unclaimed tokens.

• remainingRewards is never written in closePot. After close it still equals the pre-close residual, even though tokens have left the contract.

• ContestManager.closeContest (src/ContestManager.sol:53-60) is a thin onlyOwner wrapper around pot.closePot(). There is no second collection, sweep, or rescue on either contract.

• The 90-day gate (src/Pot.sol:50-51) and onlyOwner are the designed close path. They pass on a normal close. remainingRewards > 0 at line 53 is true after any incomplete claim.

Worked numbers matching the intended README semantics vs. what the code does:

| | Intended | Actual |
|---|---|---|
| Players / rewards | A, B, C × 100e18 | same |
| Funded pot | 300e18 | 300e18 |
| A claims | A gets 100e18 | A gets 100e18 |
| Residual at close | 200e18 | 200e18 |
| Manager cut (10%) | 20e18 to ContestManager | 20e18 to ContestManager |
| Remainder to timely claimants | 180e18 to A | 180e18 / 3 = 60e18 to A |
| Left in Pot | 0 (or dust from %) | 120e18 = 2 * 60e18 |

remainingRewards after close is still 200e18. The 120e18 on the Pot is not residual % 3 dust — it is exactly the two unclaimed claimantCuts.

If claimants is empty, the loop is a no-op: manager takes ~10% and ~90% of the residual is locked. If every player claims, the residual is 0 and the branch is skipped — the bug only fires on the common incomplete-claim case.

Note: managerCut = remainingRewards / managerCutPercent with managerCutPercent = 10 is remainingRewards / 10 (10%), which happens to match the README. That is not the issue. The issue is exclusively the divisor on line 57.

Exploitation

No malicious actor is required. The production close path locks funds whenever at least one mapped player does not claim.

1. Owner calls createContest([A, B, C], [100e18, 100e18, 100e18], token, 300e18).

• i_players.length = 3, remainingRewards = 300e18, claimants = [].

1. Owner approves the ContestManager and calls fundContest(0).

• Pot balance = 300e18.

1. A calls claimCut(). B and C never claim.

• A receives 100e18.

• remainingRewards = 200e18, claimants = [A], Pot balance = 200e18.

1. Warp until block.timestamp >= i_deployedAt + 90 days.

2. Owner calls closeContest(pot) → closePot():

• managerCut = 200e18 / 10 = 20e18 transferred to ContestManager.

• claimantCut = (200e18 - 20e18) / 3 = 60e18 transferred only to A.

• Pot balance left = 120e18.

• remainingRewards still 200e18.

• No further function can move those 120e18.

Confirmed by an executed Foundry test of this exact trace: assertEq(A's close payout, residual=180e18) failed (60e18 != 180e18). Post-close Pot balance is 120e18, not residual-mod-3 dust.

Reachable with a standard 18-decimal ERC20. No hooks, no unusual token behavior, no privileged attacker.

Impact

High — permanent loss of contest residual on the designed close path.

• Every funded contest with incomplete claims permanently strands (i_players.length - claimants.length) * claimantCut.

• Empty claimants strands ~90% of the residual after the manager cut.

• Timely claimants are underpaid relative to the README (A is entitled to the full 180e18 remainder, receives 60e18).

• Neither Pot nor ContestManager exposes a second collection or rescue. Once closePot runs, the leftover tokens are unrecoverable.

• This is the common case, not an edge case: contests routinely have no-shows. The owner calling closeContest after 90 days is the documented residual-release path.

Severity High. Confidence 95.

Recommendation

Divide the residual by the number of actual claimants, not the original player list. Guard the empty-claimants case so the remainder is not stranded.

Also consider sweeping any % claimants.length dust to the manager so the Pot balance is exactly zero after close.

→ skipped: dust-to-manager sweep, add when you want a strictly empty Pot after close.

Proof of Concept

diff --git a/test/ClosePotResidualLock.t.sol b/test/ClosePotResidualLock.t.sol
new file mode 100644
index 0000000..6a2a5b6
--- /dev/null
+++ b/test/ClosePotResidualLock.t.sol
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.20;
+
+// Attacker: none required. Any mapped player who skips claimCut, plus the owner
+// calling the designed closeContest after 90 days, locks funds forever.
+// Impact: closePot divides the residual by i_players.length but pays only
+// claimants, permanently locking (players - claimants) * claimantCut in the Pot.
+// Run: forge test --match-contract ClosePotResidualLock -vv
+
+import {Test, console} from "lib/forge-std/src/Test.sol";
+import {ContestManager} from "../src/ContestManager.sol";
+import {Pot} from "../src/Pot.sol";
+import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
+import {ERC20Mock} from "./ERC20Mock.sol";
+
+contract ClosePotResidualLock is Test {

• ContestManager manager;

• ERC20Mock token;

• address owner = makeAddr("owner");

• address playerA = makeAddr("playerA");

• address playerB = makeAddr("playerB");

• address playerC = makeAddr("playerC");

+

• uint256 constant CUT = 100e18;

• uint256 constant TOTAL = 300e18;

+

• function setUp() public {

• vm.startPrank(owner);

• manager = new ContestManager();

• token = new ERC20Mock("WETH", "WETH", owner, TOTAL);

• token.approve(address(manager), TOTAL);

• vm.stopPrank();

• }

+

• function test_closePotLocksUnclaimedShare() public {

• address[] memory players = new address[](3);

• players[0] = playerA;

• players[1] = playerB;

• players[2] = playerC;

• uint256[] memory rewards = new uint256[](3);

• rewards[0] = CUT;

• rewards[1] = CUT;

• rewards[2] = CUT;

+

• vm.startPrank(owner);

• address potAddr = manager.createContest(players, rewards, IERC20(token), TOTAL);

• manager.fundContest(0);

• vm.stopPrank();

+

• Pot pot = Pot(potAddr);

• assertEq(token.balanceOf(potAddr), TOTAL);

+

• // Only A claims in time. B and C never call claimCut.

• vm.prank(playerA);

• pot.claimCut();

• assertEq(token.balanceOf(playerA), CUT);

• assertEq(token.balanceOf(potAddr), 200e18);

• assertEq(pot.getRemainingRewards(), 200e18);

+

• vm.warp(block.timestamp + 90 days);

+

• uint256 aBeforeClose = token.balanceOf(playerA);

• uint256 managerBefore = token.balanceOf(address(manager));

+

• vm.prank(owner);

• manager.closeContest(potAddr);

+

• uint256 managerCut = token.balanceOf(address(manager)) - managerBefore;

• uint256 aClosePayout = token.balanceOf(playerA) - aBeforeClose;

• uint256 locked = token.balanceOf(potAddr);

+

• // Designed: residual 200e18 → 10% manager (20e18), rest 180e18 to timely claimants.

• // Actual: claimantCut = 180e18 / i_players.length (3) = 60e18, paid only to A.

• assertEq(managerCut, 20e18);

• assertEq(aClosePayout, 60e18);

• // README: remainder goes equally to timely claimants → A is entitled to 180e18.

• assertTrue(aClosePayout < 180e18, "A underpaid vs remaining residual");

• // Locked = (players - claimants) * claimantCut = 2 * 60e18, not dust.

• assertEq(locked, 120e18);

• // remainingRewards is never written down; no rescue path exists.

• assertEq(pot.getRemainingRewards(), 200e18);

+

• console.log("A close payout (actual):", aClosePayout);

• console.log("A close payout (intended, sole claimant):", uint256(180e18));

• console.log("Permanently locked in Pot:", locked);

• console.log("remainingRewards still:", pot.getRemainingRewards());

• }

+}