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 (the Pot's owner), which has no withdrawal function, permanently locking the cut

ghost76

Summary

The Pot is deployed by ContestManager via new Pot(...), so the Pot's Ownable owner is the ContestManager contract, not a human EOA. closePot is onlyOwner and is only reachable through ContestManager._closeContest -> pot.closePot(), meaning msg.sender inside closePot is the ContestManager contract. The manager cut is transferred to that contract, which exposes no function to withdraw or move ERC20 tokens, so the cut is permanently locked.

Description

closePot computes a manager cut and sends it with i_token.transfer(msg.sender, managerCut). The intent is for the protocol operator to receive the cut. But because of how ownership is wired, msg.sender is the ContestManager contract address, and ContestManager has no token-recovery path.

Risk

Every contest close sends the manager cut into ContestManager, where it becomes unrecoverable. This is not an edge case; it happens on the normal, intended close flow of every Pot. The protocol never actually receives the cut it is designed to collect.

Vulnerability Details

Ownership wiring:

  • ContestManager.createContest: Pot pot = new Pot(players, rewards, token, totalRewards); so Pot's constructor runs Ownable(msg.sender) with msg.sender = ContestManager. The Pot is owned by ContestManager.

  • ContestManager._closeContest: Pot pot = Pot(contest); pot.closePot(); so the caller of closePot is ContestManager.

src/Pot.sol::closePot (L54-55):

uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);   // msg.sender == ContestManager contract

ContestManager has no function that calls token.transfer or otherwise moves out received ERC20, so any tokens it holds are stuck.

Impact

  • Permanent lock of the entire manager cut on every contest (100e18 in the PoC).

  • The protocol operator never receives intended revenue.

Proof of Concept

test_HH7_manager_cut_locked_in_manager (close via ContestManager, check balances). Real forge output:

[PASS] test_HH7_manager_cut_locked_in_manager()

Assertions: token.balanceOf(address(contestManager)) == managerCut post-close (100e18); contestManager exposes no method to retrieve it.

Recommended Mitigation

Send the cut to an explicit operator address rather than msg.sender, e.g. pass a treasury/recipient at construction and transfer to it:

i_token.transfer(i_manager, managerCut);

Or add an onlyOwner token-recovery function to ContestManager so received cuts can be withdrawn.

Tools Used

Foundry (forge), manual review.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 9 hours 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!