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

Manager cut is transferred to the ContestManager contract instead of the admin, and since ContestManager has no token withdrawal function, 10% of the remaining pool is permanently frozen on every pot close

sillyrobot796

Root + Impact

Description

  • Normally, 90 days after deployment the admin calls closeContest, the pot takes managerCut = remainingRewards / 10 for the manager, and the rest is distributed to players who claimed in time.

  • The pot is deployed by the ContestManager via new Pot(...), so Ownable(msg.sender) in the pot constructor sets the pot owner to the ContestManager contract. Therefore closePot can only ever be reached through ContestManager.closeContest, meaning inside closePot the msg.sender is always the ContestManager contract — and managerCut is transferred to that contract. ContestManager contains no ERC20 withdrawal/rescue function (its only token call is the inbound transferFrom in fundContest), so the manager cut is permanently frozen.

// src/ContestManager.sol
function closeContest(address contest) public onlyOwner {
_closeContest(contest);
}
​
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
@> pot.closePot(); // inside closePot, msg.sender == address(this) == ContestManager
}
​
// src/Pot.sol
@> constructor(...) Ownable(msg.sender) { // msg.sender == ContestManager (deployer)
...
}
​
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); // recipient is the ContestManager contract

Risk

Likelihood:

  • Every close of a pot with remainingRewards > 0 routes the manager cut into the ContestManager — the only call path to closePot is ContestManager.closeContest, so this happens on every single close that has unclaimed rewards.

  • No bypass exists: Pot.owner is permanently the ContestManager (no ownership transfer is ever invoked in the codebase), so the admin can never call closePot directly to redirect the payment.

Impact:

  • The manager's 10% cut is unrecoverable on every close — e.g. in the standard testUnclaimedRewardDistribution flow (total 1000, one of two players claims) 50 WETH gets frozen inside ContestManager while the admin receives exactly 0.

  • Combined with the wrong divisor (Report V2) and the zero-claimant case (Report V3), the freeze compounds up to 100% of all funds: 100 into ContestManager + 900 stuck in the Pot when nobody claims.

Proof of Concept

PoC explanation: The test reproduces the freeze in the standard unclaimed-rewards flow. Setup: the admin deploys ContestManager, creates a 2-player pot with rewards [500, 500], and funds it with 1000 WETH. Step 1: player1 claims his 500, leaving remainingRewards = 500. Step 2: after vm.warp(91 days) the admin calls closeContest — the only possible path to closePot, so inside closePot msg.sender is necessarily the ContestManager contract. Two assertions prove the issue: (1) the ContestManager CONTRACT receives exactly 50 (= managerCut = 500 / 10); (2) the admin EOA's balance is completely unchanged (delta 0). This demonstrates that the manager cut leaves the pot but lands on an address that has no token withdrawal function, freezing 50 WETH on this close — and since this is the only close path, it happens on every close that has a remainder. Run with: forge test --match-test test_POC1_ManagerCutLockedInContestManager -vv (PASS; both assertions hold against the current code).

// forge test --match-test test_POC1_ManagerCutLockedInContestManager -> PASS
function test_POC1_ManagerCutLockedInContestManager() public {
(address[] memory players, uint256[] memory rewards) = _twoPlayers(); // [p1,p2],[500,500]
address pot = _createAndFund(players, rewards, 1000);
​
vm.prank(p1);
Pot(pot).claimCut(); // p1 +500, remainingRewards = 500
​
uint256 adminBefore = weth.balanceOf(admin);
vm.warp(91 days);
vm.prank(admin);
conMan.closeContest(pot);
​
// managerCut (50) lands in the ContestManager CONTRACT, admin gets nothing
assertEq(weth.balanceOf(address(conMan)), 50, "managerCut goes to ContestManager contract");
assertEq(weth.balanceOf(admin), adminBefore, "admin receives NOTHING");
}

Recommended Mitigation

// src/Pot.sol
- function closePot() external onlyOwner {
+ function closePot(address payoutRecipient) external onlyOwner {
...
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
- i_token.transfer(msg.sender, managerCut);
+ i_token.transfer(payoutRecipient, managerCut);
​
// src/ContestManager.sol
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
- pot.closePot();
+ pot.closePot(msg.sender); // msg.sender == admin, closeContest is onlyOwner
}

An alternative is adding an owner-only sweep(token) to ContestManager, but the explicit-recipient fix is safer as it does not create a contract-level honeypot.

⚠ Combination note: even with this fix, close remains repeatable (V4) — the owner can call closeContest 5+ times and collect 5 manager cuts, so the payout fix must ship together with the closed flag. With V2/V3 still unfixed, the claimant side still underpays/locks (225 of 450 still stranded in the pot; zero-claimant pots still strand 900).

Updates

Lead Judging Commences

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