MyCut
AI First Flight #8
Beginner FriendlyFoundry
EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

Pot::closePot never updates remainingRewards and sets no closed flag, so a repeated closeContest pays the stale manager cut and claimant bonus again, starves post-close claimers and leaves the remaining-rewards views stale

friedsocks233

closePot is not terminal + repeated payouts and stale remaining-rewards accounting

Description

  • Closing a pot should be a one-time event. The remaining pool is split once, 10% to the manager and 90% to the in-time claimants. After that, remainingRewards and the views getRemainingRewards and getContestRemainingRewards should show that nothing is left to distribute.

  • closePot reads remainingRewards but never zeroes or decrements it. The only writes are in the constructor (Pot.sol:27) and in claimCut (Pot.sol:44). It also records no closed state, and ContestManager::closeContest forwards to it without tracking anything.

  • Each later closeContest on the same pot therefore passes the 90-day check and the remainingRewards > 0 check again. It pays the same stale managerCut and claimantCut for as long as the Pot's balance covers a full round.

// src/Pot.sol
@> 49: function closePot() external onlyOwner {
50: if (block.timestamp - i_deployedAt < 90 days) {
51: revert Pot__StillOpenForClaim();
52: }
@> 53: if (remainingRewards > 0) {
54: uint256 managerCut = remainingRewards / managerCutPercent;
55: i_token.transfer(msg.sender, managerCut);
56:
57: uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
58: for (uint256 i = 0; i < claimants.length; i++) {
59: _transferReward(claimants[i], claimantCut);
60: }
61: }
62: }
// ...
76: function getRemainingRewards() public view returns (uint256) {
@> 77: return remainingRewards;
78: }
// src/ContestManager.sol
@> 53: function closeContest(address contest) public onlyOwner {
54: _closeContest(contest);
55: }

Risk

Likelihood:

  • It needs the trusted owner to call closeContest more than once on the same pot. Causes include a retried transaction, a keeper or script that re-runs, or an admin misled by getContestRemainingRewards, which still reports the pre-close amount.

  • Funds move only while the Pot still holds residue after the first close. Residue remains after any close where some players did not claim (because of the separate bonus-divisor issue) and after any close with zero claimants.

Impact:

  • Each repeat close sends another manager cut into ContestManager, which has no withdraw path, and another bonus to the existing claimants. In the PoC there are three players at 100 and only p1 claims:

    • Close 1 pays 20 + 60, leaving 120 in the Pot.

    • Close 2 pays 20 + 60 again, leaving 40.

    • Close 3 reverts atomically with ERC20InsufficientBalance(pot, 20, 60).

    p2's post-close claimCut succeeds after one close but reverts after two (ERC20InsufficientBalance(pot, 40, 100)).

  • getRemainingRewards and getContestRemainingRewards keep returning 200 after every close, which misleads off-chain consumers. Claimants are not overpaid compared with the README (p1 ends at 220 against the 280 intended). The harm is mis-routed residue and wrong accounting, not theft.

Proof of Concept

The test deploys ContestManager and the repo's test/ERC20Mock.sol. All three tests pass:

  • test_baseline_allClaim_repeatCloseIsNoOp (control): when everyone claims, remainingRewards is 0 and two closes move nothing.

  • test_repeatClose_repaysStaleCuts_staleViews_starvesLateClaimer: the scenario above. Each close is asserted to pay ContestManager 20 and p1 60. Both views stay at 200. The third close reverts atomically, p2's claim reverts, and p1's 220 is asserted to be below the README entitlement of 280.

  • test_singleClose_lateClaimStillPossible (contrast): after a single close, p2's claim of 100 still succeeds, so the second close is what starves p2.

Save the file as test/PoC_MC-04.t.sol and run from the repository root:

FOUNDRY_FFI=false forge test --match-path test/PoC_MC-04.t.sol -vv
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import {Test, console} from "lib/forge-std/src/Test.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {IERC20Errors} from "lib/openzeppelin-contracts/contracts/interfaces/draft-IERC6093.sol";
import {ContestManager} from "../src/ContestManager.sol";
import {Pot} from "../src/Pot.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
​
/// MC-04: closePot is not terminal. remainingRewards is never updated by closePot and there is
/// no closed flag, so the owner can close the same pot repeatedly; each repeat re-pays the stale
/// managerCut (into ContestManager, which has no withdraw) and the stale claimantCut, and the
/// remaining-rewards views keep reporting the pre-close figure.
contract PoC_MC04 is Test {
address admin = makeAddr("admin");
address p1 = makeAddr("p1");
address p2 = makeAddr("p2");
address p3 = makeAddr("p3");
​
ContestManager conMan;
ERC20Mock token;
​
function setUp() public {
vm.startPrank(admin);
conMan = new ContestManager();
token = new ERC20Mock("TKN", "TKN", admin, 0);
token.mint(admin, 1_000_000);
token.approve(address(conMan), type(uint256).max);
vm.stopPrank();
}
​
function _threePlayerPot() internal returns (Pot pot) {
address[] memory players = new address[](3);
players[0] = p1;
players[1] = p2;
players[2] = p3;
uint256[] memory rewards = new uint256[](3);
rewards[0] = 100;
rewards[1] = 100;
rewards[2] = 100;
​
vm.startPrank(admin);
address potAddr = conMan.createContest(players, rewards, IERC20(address(token)), 300);
conMan.fundContest(conMan.getContests().length - 1);
vm.stopPrank();
pot = Pot(potAddr);
}
​
/// Baseline: everyone claims, remainingRewards is 0, closePot pays nothing and a repeat close
/// is a harmless no-op. The bug needs residue in the pot to have any fund effect.
function test_baseline_allClaim_repeatCloseIsNoOp() public {
Pot pot = _threePlayerPot();
vm.prank(p1);
pot.claimCut();
vm.prank(p2);
pot.claimCut();
vm.prank(p3);
pot.claimCut();
assertEq(pot.getRemainingRewards(), 0);
​
vm.warp(block.timestamp + 91 days);
vm.startPrank(admin);
conMan.closeContest(address(pot));
conMan.closeContest(address(pot));
vm.stopPrank();
​
assertEq(token.balanceOf(address(pot)), 0);
assertEq(token.balanceOf(address(conMan)), 0);
assertEq(token.balanceOf(p1), 100);
}
​
/// The break: only p1 claims, the owner closes twice. Both closes succeed and pay the same
/// stale cuts; the views still read 200; a third close reverts; p2's claimCut now reverts.
function test_repeatClose_repaysStaleCuts_staleViews_starvesLateClaimer() public {
Pot pot = _threePlayerPot();
​
vm.prank(p1);
pot.claimCut();
assertEq(token.balanceOf(p1), 100);
assertEq(pot.getRemainingRewards(), 200);
assertEq(token.balanceOf(address(pot)), 200);
​
vm.warp(block.timestamp + 91 days);
​
// First close: managerCut = 200/10 = 20 to ContestManager; claimantCut = 180/3 = 60 to p1.
vm.prank(admin);
conMan.closeContest(address(pot));
assertEq(token.balanceOf(address(conMan)), 20, "close 1 manager cut");
assertEq(token.balanceOf(p1), 160, "close 1 claimant cut");
assertEq(token.balanceOf(address(pot)), 120, "pot after close 1");
​
// Stale views after a single close: still 200, though the pot holds 120.
assertEq(pot.getRemainingRewards(), 200, "Pot view stale after close");
assertEq(conMan.getContestRemainingRewards(address(pot)), 200, "CM view stale after close");
console.log("after close 1: pot balance", token.balanceOf(address(pot)));
console.log("after close 1: getContestRemainingRewards", conMan.getContestRemainingRewards(address(pot)));
​
// Second close on the same pot is accepted and pays the identical stale cuts again.
vm.prank(admin);
conMan.closeContest(address(pot));
assertEq(token.balanceOf(address(conMan)), 40, "close 2 paid manager cut again");
assertEq(token.balanceOf(p1), 220, "close 2 paid claimant cut again");
assertEq(token.balanceOf(address(pot)), 40, "pot after close 2");
assertEq(pot.getRemainingRewards(), 200, "Pot view still stale");
assertEq(conMan.getContestRemainingRewards(address(pot)), 200, "CM view still stale");
console.log("after close 2: pot balance", token.balanceOf(address(pot)));
console.log("after close 2: ContestManager balance (no withdraw path)", token.balanceOf(address(conMan)));
console.log("after close 2: p1 balance", token.balanceOf(p1));
​
// Third close: managerCut 20 fits (pot 40 -> 20), claimantCut 60 does not -> whole tx reverts.
vm.prank(admin);
vm.expectRevert(
abi.encodeWithSelector(IERC20Errors.ERC20InsufficientBalance.selector, address(pot), 20, 60)
);
conMan.closeContest(address(pot));
assertEq(token.balanceOf(address(pot)), 40, "close 3 reverted atomically");
​
// p2 still has a recorded cut of 100 but the pot only holds 40 -> claimCut reverts.
assertEq(pot.checkCut(p2), 100);
vm.prank(p2);
vm.expectRevert(
abi.encodeWithSelector(IERC20Errors.ERC20InsufficientBalance.selector, address(pot), 40, 100)
);
pot.claimCut();
​
// Context for severity: per README, p1 (sole in-time claimant) should get 90% of the
// 200 remainder = 180 bonus (total 280). After two closes p1 has 220, i.e. still underpaid
// relative to spec; the repeat close moves stranded residue, it does not overpay vs spec.
assertLt(token.balanceOf(p1), 280, "p1 still below README entitlement");
}
​
/// Without a second close, the 120 residue would remain in the pot; p2's post-close claim of
/// 100 succeeds (the code has no claim deadline). The repeat close is what removes it.
function test_singleClose_lateClaimStillPossible() public {
Pot pot = _threePlayerPot();
vm.prank(p1);
pot.claimCut();
vm.warp(block.timestamp + 91 days);
vm.prank(admin);
conMan.closeContest(address(pot));
assertEq(token.balanceOf(address(pot)), 120);
​
vm.prank(p2);
pot.claimCut();
assertEq(token.balanceOf(p2), 100);
assertEq(token.balanceOf(address(pot)), 20);
}
}

Recommended Mitigation

Make the close terminal: set a closed flag and zero remainingRewards before paying out. Combine this with the divisor fix from the bonus-distribution issue.

--- a/src/Pot.sol
+++ b/src/Pot.sol
@@ -8,6 +8,7 @@
error Pot__RewardNotFound();
error Pot__InsufficientFunds();
error Pot__StillOpenForClaim();
+ error Pot__AlreadyClosed();
address[] private i_players;
uint256[] private i_rewards;
@@ -18,6 +19,7 @@
mapping(address => uint256) private playersToRewards;
uint256 private remainingRewards;
uint256 private constant managerCutPercent = 10;
+ bool private s_closed;
constructor(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards) {
i_players = players;
@@ -47,14 +49,20 @@
}
function closePot() external onlyOwner {
+ if (s_closed) {
+ revert Pot__AlreadyClosed();
+ }
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
+ s_closed = true;
if (remainingRewards > 0) {
- uint256 managerCut = remainingRewards / managerCutPercent;
+ uint256 remaining = remainingRewards;
+ remainingRewards = 0;
+ uint256 managerCut = remaining / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
+ uint256 claimantCut = (remaining - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 1 hour ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!