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

[H-02] Manager cut sent to ContestManager contract instead of admin, permanently locking the owner's 10% fee

webrainsec

Description

When the owner closes a contest, the 10% manager cut is transferred to msg.sender inside closePot(). But msg.sender is the ContestManager contract (which relays the call), not the admin. ContestManager has no function to withdraw ERC20 tokens, so the manager cut is permanently locked.

Vulnerability Details

The call chain is: admin calls ContestManager.closeContest() which calls Pot.closePot(). Inside closePot(), msg.sender is the ContestManager contract address, not the original admin:

// src/ContestManager.sol, line 57-59
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
pot.closePot(); // @> msg.sender inside closePot() = address(this) = ContestManager
}
// src/Pot.sol, line 54-55
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut); // @> sends to ContestManager, not admin

ContestManager inherits from Ownable but has no withdraw(), rescue(), or any function that moves ERC20 tokens out. The manager cut sits in the contract balance forever.

Risk

Likelihood:

  • This triggers on every closeContest() call where unclaimed rewards exist. The owner has no way to avoid it — the only path to close a pot goes through ContestManager, so msg.sender is always the contract.

Impact:

  • The owner permanently loses their 10% manager cut from every contest. Across multiple contests, the locked amount compounds. There is no recovery mechanism.

PoC

The test creates a 1000-token contest with 2 players. Player1 claims, player2 does not. After 90 days the owner closes the pot. The manager cut (50 tokens) lands in ContestManager's balance. The admin's balance does not change.

function testExploit_ManagerCutLocked() public {
address[] memory players = new address[](2);
players[0] = player1;
players[1] = player2;
​
uint256[] memory rewards = new uint256[](2);
rewards[0] = 500e18;
rewards[1] = 500e18;
​
vm.startPrank(admin);
address contest = conMan.createContest(players, rewards, IERC20(token), 1000e18);
conMan.fundContest(0);
vm.stopPrank();
​
// Player1 claims, player2 does not
vm.prank(player1);
Pot(contest).claimCut();
​
uint256 adminBefore = token.balanceOf(admin);
uint256 conManBefore = token.balanceOf(address(conMan));
​
// Close after 90 days
vm.warp(block.timestamp + 91 days);
vm.prank(admin);
conMan.closeContest(contest);
​
uint256 adminAfter = token.balanceOf(admin);
uint256 conManAfter = token.balanceOf(address(conMan));
​
// Admin received ZERO manager cut
assertEq(adminAfter - adminBefore, 0, "Admin got 0 - manager cut went elsewhere");
// ContestManager got the manager cut with no way to withdraw
assertEq(conManAfter - conManBefore, 50e18, "50 tokens locked in ContestManager");
}

Recommendations

Pass the admin address through to closePot() so the manager cut goes directly to the owner. This avoids the msg.sender mismatch entirely.

// ContestManager.sol
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
- pot.closePot();
+ pot.closePot(msg.sender);
}
​
// Pot.sol
- function closePot() external onlyOwner {
+ function closePot(address manager) external onlyOwner {
...
- i_token.transfer(msg.sender, managerCut);
+ i_token.transfer(manager, managerCut);
Updates

Lead Judging Commences

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