Root cause: `Pot` is `Ownable(msg.sender)` but is always deployed by the factory, so the manager cut is transferred to the `ContestManager` contract. Impact: the manager never receives his cut and it is locked there with no way out
Root cause: Pot is Ownable(msg.sender) but is always deployed by the factory, so the manager cut is transferred to the ContestManager contract. Impact: the manager never receives his cut and it is locked there with no way out
Description
The README lists the manager as a human role: the Owner/Admin creates pots, funds them and closes them once the claim period has elapsed, at which point he "takes a cut of the remaining pool". That cut is his payment, so it has to reach the address that owns the system.
Pot is declared contract Pot is Ownable(msg.sender), but a Pot is never deployed by a person. It is always constructed inside ContestManager.createContest, so at construction time msg.sender is the factory contract, and the factory becomes the owner of every pot. closePot is onlyOwner, which means the only caller that can ever reach it is ContestManager, and inside the function the cut is sent to msg.sender, which is that same factory contract rather than the human who called closeContest.
ContestManager holds no function that sends ERC20 anywhere. Its whole surface is createContest, fundContest, closeContest, _closeContest and four view getters, and fundContest only pulls tokens in. No sweep, no rescue, no call forwarder, and it grants no allowance, so tokens that land there cannot be moved by anyone.
Deployed directly by a person the very same contract pays the cut to that person, which is what Ownable(msg.sender) reads like it was written for. Only the factory route redirects it.
Risk
Likelihood:
-
It happens on every single close of every pot, because the ownership is fixed at construction and the transfer target is derived from it.
-
No unusual configuration is involved. The manager follows the documented flow and still receives nothing.
Impact:
-
The manager is paid zero for every contest he runs, while the protocol reports the cut as taken. On a 1000 token contest with half unclaimed, 50 tokens leave the pot and none of them reach him.
-
Those tokens are permanently frozen on the
ContestManageraddress. Every contest ever closed adds to the pile and none of it is recoverable.
Proof of Concept
The manager here is the honest owner and the only privileged actor, there is no attacker. Two players are registered for 500 each, one claims, and after the window the manager closes the contest expecting his 10 percent. His balance does not move and the tokens appear on the factory instead, with no path back. Run with forge test --match-test test_R2 -vv.
Output:
Recommended Mitigation
Send the cut to the address that actually represents the manager rather than to whoever called the contract. Since the factory is the owner of the pot, the human owner is one hop away and can be read directly:
A cleaner alternative is to pass the intended manager into the Pot constructor and store it as an immutable, which removes the dependency on who deployed the contract. Either way, add a way for ContestManager to forward tokens it holds, so cuts already trapped by earlier closes are not lost.
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.