[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.
# Finding: Owner Cut Is Sent to ContestManager and Locked Forever
## Severity
Critical
## Summary
The ownership model between the Pot and ContestManager contracts is misaligned. Each new Pot is created via the ContestManager, but the Pot inherits Ownable(msg.sender), where msg.sender is the ContestManager contract itself rather than the original user or intended owner. As a result, when the Pot closes and transfers its owner cut, the tokens are sent to the ContestManager instead of the actual protocol owner or intended recipient. Those funds are then trapped because the ContestManager does not implement a withdrawal mechanism.
## Affected Contracts
- src/Pot.sol
- src/ContestManager.sol
## Root Cause
The root cause is an ownership assignment bug in the Pot constructor:
```solidity
contract Pot is Ownable(msg.sender) {
// ...
}
```
The Pot is deployed from inside ContestManager.createContest():
```solidity
function createContest(...) public onlyOwner returns (address) {
Pot pot = new Pot(players, rewards, token, totalRewards);
// ...
}
```
In this context, msg.sender inside the Pot constructor resolves to the ContestManager contract address, not the user who initiated the creation flow. The result is that the ContestManager becomes the owner of every Pot it creates.
## Vulnerability Mechanism
When a contest is closed, closeContest() calls the Pot's closePot() function:
```solidity
function closeContest(address contest) public onlyOwner {
Pot(contest).closePot();
}
```
And inside Pot.closePot():
```solidity
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
// ...
}
```
Because the owner of the Pot is the ContestManager, the transfer goes to the ContestManager contract itself. Since the ContestManager has no way to recover or redistribute those tokens, the owner cut is effectively stuck in the contract.
## Impact
- Loss of protocol treasury / owner fees
- Rewards can become permanently inaccessible
- Contest lifecycle can lock funds in the manager contract for the lifetime of the protocol
This is a critical issue because the funds are not just delayed — they are unrecoverable without an explicit contract-level withdrawal path.
## Proof of Concept
The following test demonstrates the issue:
```solidity
function testOwnerCutStuckInContestManager() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(
players,
rewards,
IERC20(ERC20Mock(weth)),
100
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
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);
assertGt(conManBalanceAfter, conManBalanceBefore);
console.log("contest manager balance after:", conManBalanceAfter);
}
```
Observed output:
```text
contest manager balance before: 0
contest manager balance after: 10
```
This proves the manager cut is being sent to the ContestManager contract and remains there.
## Recommendation
Use a proper ownership handoff pattern so the Pot's owner is the real contest creator or a trusted admin, rather than the ContestManager contract itself.
Recommended approach:
- Deploy Pot with an explicit owner parameter, such as `new Pot(owner, players, rewards, token, totalRewards)`
- Or set ownership after deployment using a dedicated initializer or constructor argument
- Add a withdrawal function to the ContestManager for ERC20 recovery if the platform architecture requires the manager to receive fees
Example approach:
```solidity
function claimStuckedERC20(address tkn, address to, uint256 amount) external onlyOwner {
(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"
);
}
```
## Severity Rationale
This is a direct loss-of-funds issue. Once the manager cut is routed to the ContestManager contract, it is not recoverable without a new protocol-level rescue path. The issue is deterministic, exploitable by normal contest closure flow, and can permanently trap deposited rewards.
Lead Judging Commences
[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' ); ```
Rewards breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Judging
Submissions are being reviewed by our AI judge. Results will be available in a few minutes.
View all submissionsRewards Distribution
The contest is complete and the rewards are being distributed.