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

Manager cut in `Pot.closePot` is transferred to the `ContestManager` contract instead of the admin, and `ContestManager` has no token-withdraw function — the cut is permanently locked

cdude

Root + Impact

Description

  • Pot.closePot is onlyOwner, where the Pot's owner is set to msg.sender of its constructor — the ContestManager contract that deployed it. When the admin calls ContestManager.closeContest, the resulting call into pot.closePot() has msg.sender == ContestManager address, not the admin's address.

  • closePot then runs i_token.transfer(msg.sender, managerCut), sending the 10% manager fee to the ContestManager contract. ContestManager has no withdraw, no admin token-rescue, and no fallback — every manager cut from every Pot accumulates in ContestManager and is permanently unrecoverable by the admin.

// src/Pot.sol :: closePot
if (remainingRewards > 0) {
@> uint256 managerCut = remainingRewards / managerCutPercent;
@> i_token.transfer(msg.sender, managerCut); // msg.sender == ContestManager contract, NOT the admin
// ...
}
// src/ContestManager.sol — no withdraw / rescue function exists
contract ContestManager is Ownable {
function createContest(...) public onlyOwner returns (address) { ... }
function fundContest(...) public onlyOwner { ... }
function closeContest(...) public onlyOwner { _closeContest(...); }
// ...
// <no function ever sends tokens out of this contract>
}

Risk

Likelihood:

  • Triggered on every single closePot call when remainingRewards > 0 — the standard, expected protocol path.

  • No attacker, no race condition, no special precondition required. Happens by default.
    Impact:

  • The entire manager cut from every Pot is permanently stuck in the ContestManager contract.

  • The admin (intended fee recipient) receives zero tokens despite the protocol's design and the 10% cut accounting.

  • Accumulates across all Pots managed — the loss scales linearly with protocol usage.

Proof of Concept

function test_managerCutSentToContestManagerWithNoWithdraw() public {
Pot pot = _setupPot5Players1000Each();
vm.warp(block.timestamp + 91 days);
uint256 ownerBalBefore = token.balanceOf(owner);
uint256 managerBalBefore = token.balanceOf(address(manager));
vm.prank(owner);
manager.closeContest(address(pot));
// Admin received NOTHING — the cut went to the ContestManager contract.
assertEq(token.balanceOf(owner), ownerBalBefore);
assertEq(token.balanceOf(address(manager)) - managerBalBefore, 500e18);
}

Run with forge test --match-test test_managerCutSentToContestManagerWithNoWithdraw -vvv. Test passes.

Recommended Mitigation

Forward the admin address explicitly through closePot, or query ContestManager.owner() for the recipient.

Option A (forward via call):

- function closePot() external onlyOwner {
+ function closePot(address feeRecipient) 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(feeRecipient, managerCut);
// ...
}
}
// src/ContestManager.sol
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
- pot.closePot();
+ pot.closePot(owner()); // pay the admin directly
}

Option B (rescue path on ContestManager):

+function rescueERC20(IERC20 t, uint256 amount) external onlyOwner {
+ t.transfer(owner(), amount);
+}

Option A is preferred because it makes the intended recipient explicit at the protocol level.

Updates

Lead Judging Commences

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