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

Manager Cut Sent to ContestManager Address with No Withdrawal Function — Permanently Locked

mkmustapha1

Root + Impact

Description

  • closePot() is designed to send the manager's 10% cut to the owner of the Pot contract as compensation for administering the distribution.

  • When ContestManager deploys a new Pot via createContest(), the Pot constructor runs with msg.sender = ContestManager, making ContestManager the owner of every Pot it creates. When closePot() executes i_token.transfer(msg.sender, managerCut), msg.sender is the ContestManager contract — not the human admin. Since ContestManager has no withdrawal function, the manager cut is permanently locked inside it with no recovery path.

// ContestManager.sol
function createContest(...) public onlyOwner returns (address) {
// @> new Pot() called here — msg.sender inside Pot constructor
// @> = address(ContestManager), NOT the human admin
// @> ContestManager becomes the owner of every Pot it creates
Pot pot = new Pot(players, rewards, token, totalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}
​
// Pot.sol
// @> Ownable(msg.sender) assigns owner = ContestManager address
contract Pot is Ownable(msg.sender) { ... }
​
function closePot() external onlyOwner {
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
​
// @> msg.sender here = ContestManager (the caller of closePot)
// @> tokens transferred TO ContestManager contract
// @> ContestManager has zero withdrawal or rescue functions
// @> managerCut is permanently stuck with no recovery
i_token.transfer(msg.sender, managerCut);
}
}

Risk

Likelihood: High

  • This occurs on every single closeContest() call without exception — the ownership chain is always ContestManager → Pot, so msg.sender in closePot() is always ContestManager

  • No special conditions required — normal protocol usage guarantees the bug is triggered every time a pot is closed

Impact: High

  • The manager receives zero compensation despite the protocol explicitly reserving a 10% cut for them on every pot closure

  • All manager cut tokens across every pot are permanently locked in ContestManager — the total loss scales with the number of pots and their unclaimed reward pools

Proof of Concept

The test below proves that after closeContest() is called, the ContestManager contract balance increases by exactly the manager cut amount — confirming the tokens went to the contract, not the human admin. The human admin balance is unchanged.

function test_ManagerCutLockedInContestManager() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(
players, rewards, IERC20(ERC20Mock(weth)), totalRewards
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
vm.warp(block.timestamp + 91 days);
​
uint256 conManBalanceBefore = ERC20Mock(weth).balanceOf(conMan);
uint256 adminBalanceBefore = ERC20Mock(weth).balanceOf(user);
​
vm.prank(user);
ContestManager(conMan).closeContest(contest);
​
uint256 conManBalanceAfter = ERC20Mock(weth).balanceOf(conMan);
uint256 adminBalanceAfter = ERC20Mock(weth).balanceOf(user);
​
// ContestManager received the manager cut — tokens are stuck here
assertGt(conManBalanceAfter, conManBalanceBefore);
​
// Human admin received nothing despite being the intended recipient
assertEq(adminBalanceBefore, adminBalanceAfter);
}

Recommended Mitigation

Pass the human owner's address into the Pot constructor explicitly and store it as the manager recipient, rather than relying on msg.sender inside closePot(). This decouples ownership (used for access control) from the payment recipient address.

// Pot.sol
+ address private immutable i_manager;
​
- constructor(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards) {
+ constructor(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards, address manager) {
+ i_manager = manager;
// ... rest of constructor
}
​
function closePot() external onlyOwner {
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
- i_token.transfer(msg.sender, managerCut);
+ i_token.transfer(i_manager, managerCut);
}
}
​
// ContestManager.sol
function createContest(...) public onlyOwner returns (address) {
- Pot pot = new Pot(players, rewards, token, totalRewards);
+ Pot pot = new Pot(players, rewards, token, totalRewards, msg.sender);
}
Updates

Lead Judging Commences

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