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

Manager cut tokens are trapped in ContestManager without withdrawal mechanism

zmettalz

No withdrawal mechanism funds trapped

Description

  • When closePot() is called after 90 days, it transfers a manager cut fee to msg.sender, which is intended to be the protocol fee recipient.

  • The ContestManager contract calls closePot() on behalf of the system, making msg.sender equal to the ContestManager address rather than the intended beneficiary, causing protocol fees to accumulate in ContestManager with no withdrawal mechanism.

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); // @audit msg.sender is ContestManager, not fee recipient
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

Risk

Likelihood:

  • This occurs every time closePot() is called, as ContestManager is always the caller in the normal protocol flow.

  • The protocol architecture requires ContestManager to call closePot() after the 90-day period, making this behavior unavoidable without code changes.

Impact:

  • Protocol fees accumulate in ContestManager with each pot closure and become inaccessible if no withdrawal function exists.

  • Violates the explicit protocol invariant that ContestManager should never hold tokens, potentially breaking integrations or assumptions that depend on this constraint.

  • The locked value grows proportionally with the number of contests (e.g., with 10% manager cut and 1,000 token pots, 100 tokens are trapped per contest).

Proof of Concept

The following scenario demonstrates how manager cut tokens become trapped in ContestManager during normal protocol operation:

  1. A pot is created with 1,000 tokens total rewards and two players

  2. Player 0x10000 claims their 500 token share during the normal claim period

  3. After 90 days, ContestManager calls closeContest() which triggers closePot()

  4. Inside closePot(), the manager cut is calculated as 50 tokens (500 remaining / 10)

  5. The transfer executes with msg.sender as the recipient, which is ContestManager

  6. The 50 tokens meant for protocol fees are now stuck in ContestManager

// Initial state
ContestManager balance: 0 tokens
Pot remainingRewards: 500 tokens (one player already claimed 500)
​
// ContestManager.closeContest() calls Pot.closePot()
// Inside closePot():
managerCut = 500 / 10 = 50 tokens
i_token.transfer(msg.sender, 50) // msg.sender = ContestManager address
​
// Final state
ContestManager balance: 50 tokens ❌ (should be 0)
Pot balance: 225 tokens
Assertion fails: "ContestManager holds tokens: 50 != 0"

The test's assertion explicitly verifies that ContestManager should never hold tokens, confirming this accumulation violates the protocol's design invariant.

Recommended Mitigation

The root cause is that closePot() transfers the manager cut to msg.sender, assuming it is the fee recipient. However, in the protocol architecture, ContestManager is the caller, not the intended recipient. The solution is to store a dedicated fee recipient address and transfer manager cuts directly to that address instead of relying on msg.sender.

Add a feeRecipient state variable that is set during Pot deployment and use it as the transfer destination for manager cuts:

+ address public immutable feeRecipient;
​
+ constructor(address _feeRecipient, /* other params */) {
+ feeRecipient = _feeRecipient;
+ // ...
+ }
​
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);
+ i_token.transfer(feeRecipient, managerCut);
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

This ensures manager cuts are sent to the correct recipient regardless of who calls the function, preventing token accumulation in ContestManager and maintaining the protocol invariant.

Updates

Lead Judging Commences

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