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

Pot.closePot() sends the manager cut to the ContestManager contract which has no withdrawal function, permanently locking the manager's fees

markandeaidc

Description

Pot.closePot() transfers the manager's cut to msg.sender. Because Pot is deployed by ContestManager (new Pot(...) inside createContest) and Pot inherits Ownable(msg.sender), the only address able to call closePot() is the ContestManager contract itself — reached through ContestManager.closeContest() → _closeContest() → pot.closePot(). Therefore msg.sender inside closePot() is always the ContestManager contract, never the human admin.

// src/Pot.sol — closePot()
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); // msg.sender == ContestManager contract
...
}
}

ContestManager exposes no way to move tokens out. Its complete external surface is createContest, fundContest, getContests, getContestTotalRewards, getContestRemainingRewards, closeContest. There is no withdraw, sweep, or rescue function anywhere in the contract.

The README states "the manager takes a cut of the remaining pool", and the Actors section defines the manager as the Owner/Admin. That cut is instead credited to a contract with no withdrawal path, so it is permanently inaccessible to everyone — including the admin who owns the contract.

This is a distinct root cause from the incorrect claimant divisor in the same function: even with the divisor corrected, the manager cut would still be routed to an address that cannot forward it.

Risk

Likelihood: HIGH — deterministic. Every successful closePot() with remainingRewards > 0 routes the cut to the ContestManager contract. No special state, timing, or actor is required.

Impact: HIGH — permanent freezing of funds. The manager's entire fee is unrecoverable. In the scenario below, 6 tokens (10% of the 60-token remainder) are locked in ContestManager forever, and the admin's balance is unchanged by the close.

Severity: HIGH (High impact × High likelihood)

Proof of Concept

The test deploys the real ContestManager and Pot, funds the pot with 100 tokens for 5 players (20 each), lets 2 players claim inside the window, then closes the contest after 90 days. It compares the admin's balance before and after the close and shows the cut landing on the ContestManager contract instead.

Steps:

Place as test/PotManagerCutRecipientPoC.t.sol and run:

forge test --match-contract PotManagerCutRecipientPoC -vvv
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
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 PotManagerCutRecipientPoC is Test {
address owner = makeAddr("owner");
address p1 = makeAddr("p1");
address p2 = makeAddr("p2");
address p3 = makeAddr("p3");
address p4 = makeAddr("p4");
address p5 = makeAddr("p5");
​
uint256 constant PER_PLAYER = 20;
uint256 constant TOTAL = 100;
​
ContestManager manager;
ERC20Mock token;
address pot;
​
function setUp() public {
vm.startPrank(owner);
manager = new ContestManager();
token = new ERC20Mock("MOCK", "MCK", owner, 1000);
token.approve(address(manager), 1000);
​
address[] memory players = new address[](5);
uint256[] memory rewards = new uint256[](5);
players[0] = p1; players[1] = p2; players[2] = p3; players[3] = p4; players[4] = p5;
for (uint256 i = 0; i < 5; i++) rewards[i] = PER_PLAYER;
​
pot = manager.createContest(players, rewards, IERC20(address(token)), TOTAL);
manager.fundContest(0);
vm.stopPrank();
}
​
function test_managerCut_locked_in_ContestManager() public {
vm.prank(p1);
Pot(pot).claimCut();
vm.prank(p2);
Pot(pot).claimCut();
​
uint256 adminBefore = token.balanceOf(owner);
console.log("admin balance before close:", adminBefore);
assertEq(adminBefore, 900);
​
vm.warp(block.timestamp + 90 days);
vm.prank(owner);
manager.closeContest(pot);
​
uint256 adminAfter = token.balanceOf(owner);
uint256 managerContractBalance = token.balanceOf(address(manager));
​
console.log("admin balance after close:", adminAfter);
console.log("ContestManager contract balance:", managerContractBalance);
​
assertEq(managerContractBalance, 6, "cut routed to the contract, not the admin");
assertEq(adminAfter, adminBefore, "admin received no cut");
assertGt(managerContractBalance, 0, "manager fees permanently locked");
}
}

Actual output:

[PASS] test_managerCut_locked_in_ContestManager() (gas: 218184)
admin balance before close: 900
admin balance after close: 900
ContestManager contract balance: 6
Suite result: ok. 1 passed; 0 failed; 0 skipped

The admin's balance is identical before and after closing the contest, while 6 tokens accumulate in ContestManager with no withdrawal path.

Recommended Mitigation

Send the cut to the human owner of the ContestManager rather than to the calling contract.

// 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);
+ i_token.transfer(Ownable(msg.sender).owner(), managerCut);
...
}
}

A cleaner alternative is to have ContestManager._closeContest() pass the fee recipient into closePot(address feeRecipient) and transfer to that address, avoiding the cross-contract owner() read.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 16 minutes ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-01] Owner Cut Stuck in `ContestManager`

## Description When `closeContest` function in the `ContestManager` contract is called, `pot` sends the owner's cut to the `ContestManager` itself, with no mechanism to withdraw these funds. ## Vulnerability Details: Relevant code - [Pot](https://github.com/Cyfrin/2024-08-MyCut/blob/main/src/Pot.sol#L7) [ContestManager](https://github.com/Cyfrin/2024-08-MyCut/blob/main/src/ContestManager.sol#L16-L26) The vulnerability stems from current ownership implementation between the `Pot` and `ContestManager` contracts, leading to funds being irretrievably locked in the `ContestManager` contract. 1. **Ownership Assignment**: When a `Pot` contract is created, it assigns `msg.sender` as its owner: ```solidity contract Pot is Ownable(msg.sender) { ... } ``` 2. **Contract Creation Context**: The `ContestManager` contract creates new `Pot` instances through its `createContest` function: ```solidity function createContest(...) public onlyOwner returns (address) { Pot pot = new Pot(players, rewards, token, totalRewards); ... } ``` In this context, `msg.sender` for the new `Pot` is the `ContestManager` contract itself, not the external owner who called `createContest`. 3. **Unintended Ownership**: As a result, the `ContestManager` becomes the owner of each `Pot` contract it creates, rather than the intended external owner. 4. **Fund Lock-up**: When `closeContest` is called (after the 90-day contest period), it triggers the `closePot` function: ```solidity function closeContest(address contest) public onlyOwner { Pot(contest).closePot(); } ``` The `closePot` function sends the owner's cut to its caller. Since the caller is `ContestManager`, these funds are sent to and locked within the `ContestManager` contract. 5. **Lack of Withdrawal Mechanism**: The `ContestManager` contract does not include any functionality to withdraw or redistribute these locked funds, rendering them permanently inaccessible. This ownership misalignment and the absence of a fund recovery mechanism result in a critical vulnerability where contest rewards become permanently trapped in the `ContestManager` contract. ## POC In existing test suite, add following test ```solidity function testOwnerCutStuckInContestManager() public mintAndApproveTokens { vm.startPrank(user); contest = ContestManager(conMan).createContest( players, rewards, IERC20(ERC20Mock(weth)), 100 ); ContestManager(conMan).fundContest(0); vm.stopPrank(); // Fast forward 91 days vm.warp(block.timestamp + 91 days); uint256 conManBalanceBefore = ERC20Mock(weth).balanceOf(conMan); console.log("contest manager balance before:", conManBalanceBefore); vm.prank(user); ContestManager(conMan).closeContest(contest); uint256 conManBalanceAfter = ERC20Mock(weth).balanceOf(conMan); // Assert that the ContestManager balance has increased (owner cut is stuck) assertGt(conManBalanceAfter, conManBalanceBefore); console.log("contest manager balance after:", conManBalanceAfter); } ``` run `forge test --mt testOwnerCutStuckInContestManager -vv` in the terminal and it will return following output: ```js [⠊] Compiling... [⠑] Compiling 1 files with Solc 0.8.20 [⠘] Solc 0.8.20 finished in 1.66s Compiler run successful! Ran 1 test for test/TestMyCut.t.sol:TestMyCut [PASS] testOwnerCutStuckInContestManager() (gas: 810988) Logs: User Address: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D Contest Manager Address 1: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353 Minting tokens to: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D Approved tokens to: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353 contest manager balance before: 0 contest manager balance after: 10 Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.51ms (1.31ms CPU time) ``` ## Impact Loss of funds for the protocol / owner ## Recommendations Add a claimERC20 function `ContestManager` to solve this issue. ```solidity function claimStuckedERC20(address tkn, address to, uint256 amount) external onlyOwner { // bytes4(keccak256(bytes('transfer(address,uint256)'))); (bool success, bytes memory data) = tkn.call(abi.encodeWithSelector(0xa9059cbb, to, amount)); require( success && (data.length == 0 || abi.decode(data, (bool))), 'ContestManager::safeTransfer: transfer failed' ); ```

Support

FAQs

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

Give us feedback!