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

`ContestManager` does not implement a withdrawal function, leaving owner profits permanently trapped.

jfornells

Root + Impact

ContestManager acts as owner of all Pot contracts and receives the 10% manager cut when closePot() is called. Since ContestManager has no withdraw function, all accumulated profits are permanently inaccessible.

Field Value
Severity High
Likelihood High

Description

closePot() transfers the manager cut to msg.sender. When called via ContestManager::closeContest(), msg.sender inside Pot is the ContestManager address — not the actual protocol owner. Since ContestManager does not implement any token withdrawal mechanism, every manager cut from every Pot is permanently locked.

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, not the actual owner
​
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

Risk

Likelihood: Triggered every time closeContest() is called, which is the normal end-of-lifecycle operation for every Pot.

Impact: 10% of all unclaimed rewards from every Pot are permanently trapped. With N deployed contests each holding T total rewards and an average unclaimed rate of U, the total trapped = N × T × U × 10%. In the PoC with a single Pot of 4 WETH and 1 unclaimed, 1e17 WETH is immediately inaccessible. Across multiple contests this accumulates indefinitely with no recovery path.

Proof of Concept

Setup: 4 players, 1 WETH each. 3 claim. Owner closes after 90 days.
ContestManager receives 1e17 WETH but the actual owner balance remains 0 — no withdrawal is possible.

function test_The_Owners_Profits_AreTrapped_In_ContestManager() public {
// There are 4 players, 1 weth each
// 3 players call claimCut()
address[3] memory pClaim = [player1, player3, player4];
for (uint256 i; i < pClaim.length; i++) {
vm.prank(pClaim[i]);
pot.claimCut();
}
// Advance past 90 days to allow closePot()
vm.warp(block.timestamp + 90 days);
// Owner closes the pot — manager cut (1e17) is sent to ContestManager, not ownerContest
vm.prank(ownerContest);
contest.closeContest(address(pot));
// ContestManager holds 1e17 WETH — profits are trapped here
console.log("ContestManager weth balance:", weth.balanceOf(address(contest)));
// Actual owner received nothing — no withdraw function exists in ContestManager
assertEq(weth.balanceOf(ownerContest), 0);
}
Logs:
ContestManager weth balance: 100000000000000000

Recommended Mitigation

Add a withdraw function to ContestManager restricted to the owner.

+ function withdraw(IERC20 token) external onlyOwner {
+ token.transfer(msg.sender, token.balanceOf(address(this)));
+ }
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 18 days 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!