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

Ownable Inheritance Routes Manager Cut to Contract Address — Protocol Fees Permanently Locked

kivanov

Ownable Inheritance Routes Manager Cut to Contract Address — Protocol Fees Permanently Locked

Description

The protocol charges a 10% management fee on unclaimed rewards when a Pot is closed via closePot(). This fee is intended to compensate the human administrator for running the contest. The tokens should be sent to the admin's externally-owned account (EOA).

However, the Pot contract inherits Ownable(msg.sender), and since Pots are deployed by ContestManager.createContest() via new Pot(...), the msg.sender during construction is the ContestManager contract address — not the human admin. When closePot() is called (also by ContestManager via _closeContest()), msg.sender is again the ContestManager contract. The manager cut is therefore transferred to the ContestManager contract, which has no ERC20 withdrawal function, permanently locking the protocol's revenue.

// Pot.sol L7 — Ownable sets owner to deployer
@> contract Pot is Ownable(msg.sender) {
// When deployed via ContestManager.createContest(), msg.sender = ContestManager
}
​
// Pot.sol L49-55
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); // msg.sender = ContestManager contract
}
}
​
// ContestManager.sol L57-60
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
@> pot.closePot(); // msg.sender for closePot() = ContestManager (this)
}

Risk

Likelihood: High

  • This occurs on every single closePot() invocation — there is no alternative code path. The Ownable inheritance is hardcoded at the contract level and cannot be changed after deployment.

Impact: High

  • 100% of protocol management fees are permanently locked. The ContestManager has no withdraw(), sweep(), or selfdestruct() function.

  • This breaks the protocol's revenue model entirely — the admin receives zero compensation for managing contests.

Severity: High

Proof of Concept

The admin deploys ContestManager, creates a contest with 1000 USDC, funds it, and waits 90 days. After closing the Pot, the 10% manager cut (e.g., 30 USDC from 300 remaining) is transferred to the ContestManager contract. The admin checks their balance and finds zero received. The ContestManager contract holds 30 USDC with no way to extract it.

function test_H03_manager_cut_locked() public {
// Setup: deploy ContestManager, create and fund a contest
ContestManager manager = new ContestManager();
// ... create contest with 1000 USDC, 10 players
​
// Fund and let 7 players claim (300 USDC remains)
manager.fundContest(0);
// ... 7 players claim ...
​
// Close after 90 days
vm.warp(block.timestamp + 91 days);
manager.closeContest(address(pot));
​
// Manager cut = 300/10 = 30 USDC → sent to ContestManager contract
uint256 lockedInManager = token.balanceOf(address(manager));
assertGt(lockedInManager, 0, "Manager cut locked in contract");
​
// Admin received nothing
uint256 adminBalance = token.balanceOf(admin);
assertEq(adminBalance, 0, "Admin received zero — fees lost");
}

Recommended Mitigation

The fix passes the admin's address to the Pot constructor and sends the manager cut directly to the human admin instead of msg.sender. This decouples the fee recipient from the Ownable inheritance chain.

// Pot.sol
+ address public immutable i_admin;
​
- constructor(...) Ownable(msg.sender) {
+ constructor(..., address admin) Ownable(msg.sender) {
+ i_admin = admin;
}
​
function closePot() external onlyOwner {
// ...
- i_token.transfer(msg.sender, managerCut);
+ i_token.transfer(i_admin, managerCut);
}
Updates

Lead Judging Commences

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