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

Manager cut in Pot.closePot is sent to msg.sender (the ContestManager contract), which cannot withdraw ERC20 tokens, locking the cut permanently

tray198

Root cause + Impact

Pot::closePot is external onlyOwner, and the owner of every Pot is the ContestManager contract, because Pot is Ownable(msg.sender) and is deployed inside ContestManager::createContest. The only path that can reach closePot is ContestManager::closeContest -> _closeContest -> pot.closePot(), so inside closePot the value of msg.sender is always the ContestManager contract address. The line i_token.transfer(msg.sender, managerCut) therefore sends the 10% manager cut to the ContestManager contract, which exposes no function capable of moving ERC20 tokens out. The manager cut is permanently locked on every close.

Description

  • Normal behavior: after the 90-day window, the manager should receive a 10% cut of the leftover rewards at a controllable address (the admin EOA).

  • The issue: closePot pays the cut to msg.sender. Since closePot is onlyOwner and the owner is the ContestManager contract, msg.sender is deterministically that contract, not the admin. ContestManager has only createContest, fundContest, closeContest and view getters — none can transfer out an arbitrary ERC20 balance. The received cut is therefore trapped in ContestManager forever.

// Pot.sol
function closePot() external onlyOwner {
...
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
@> i_token.transfer(msg.sender, managerCut); // msg.sender is the ContestManager contract, which cannot withdraw
...
}
}
// ContestManager.sol has NO function that transfers ERC20 out of itself.

Risk

Likelihood:

  • Occurs on every close of a pot that has leftover rewards, which is the standard end-of-lifecycle path — the cut is routed to and trapped in ContestManager every time.

  • onlyOwner guarantees the caller is the ContestManager contract, so msg.sender can never be the admin EOA; the misrouting is deterministic, not conditional.

Impact:

  • The manager cut (10% of the remaining rewards) is permanently locked inside ContestManager with no recovery path — an unrecoverable loss of protocol funds on every close.

Proof of Concept

  1. Admin calls ContestManager::createContest(...), which does new Pot(...); therefore Pot.owner() == address(ContestManager).

  2. After 90 days, admin calls ContestManager::closeContest(pot), which internally calls pot.closePot().

  3. Inside closePot, msg.sender == address(ContestManager), so i_token.transfer(address(ContestManager), managerCut) moves the cut into the ContestManager contract.

  4. ContestManager has no function to transfer that ERC20 balance out, so the cut is stuck.

// e.g. remainingRewards = 800 tokens at close:
uint256 managerCut = 800 / 10; // = 80 tokens
// transferred to address(ContestManager); no withdraw path exists -> 80 tokens locked forever

Recommended Mitigation

Pay the cut to the ContestManager owner (the admin EOA) rather than to msg.sender, or pass an explicit recipient:

- i_token.transfer(msg.sender, managerCut);
+ i_token.transfer(Ownable(msg.sender).owner(), managerCut);

Alternatively, add an onlyOwner token-rescue/withdraw function to ContestManager so any tokens it receives can be recovered.

Updates

Lead Judging Commences

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