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

`Pot::closePot` sends the manager cut to the `ContestManager` contract, which has no way to withdraw it, locking the cut forever

summus

Root + Impact

Description

  • When a Pot is closed, the manager (the admin) receives 10% of the remaining rewards as a fee.

  • Pot inherits Ownable(msg.sender) and is deployed with new Pot(...) inside ContestManager::createContest, so every Pot's owner is the ContestManager contract, not the admin. closePot is onlyOwner and can only be reached through ContestManager::closeContest, so msg.sender in closePot is always ContestManager, and the cut is transferred there. ContestManager has no function that moves ERC20 tokens out, so the cut can never be recovered.

// Pot.sol
@> contract Pot is Ownable(msg.sender) { // msg.sender == ContestManager
...
function closePot() external onlyOwner {
...
uint256 managerCut = remainingRewards / managerCutPercent;
@> i_token.transfer(msg.sender, managerCut); // goes to ContestManager
...
}
// ContestManager.sol
function createContest(...) public onlyOwner returns (address) {
@> Pot pot = new Pot(players, rewards, token, totalRewards);
...
}
​
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
@> pot.closePot();
}
// no withdraw / sweep / rescue function anywhere in ContestManager

Risk

Likelihood:

  • Every time a Pot is closed with unclaimed rewards (remainingRewards > 0), which is the only case where a manager cut exists.

  • No other path exists: the admin cannot call Pot::closePot directly because the admin is not the Pot's owner.

Impact:

  • The admin never receives the 10% fee from any Pot. The protocol's whole revenue is permanently lost.

  • The tokens accumulate in ContestManager across all contests with no recovery path.

Proof of Concept

player2 claims 35 out of 100, and the admin closes the Pot after 90 days. The 6-token cut (65 / 10) ends up in ContestManager, and the admin's balance does not change.

function testManagerCutStuckInContestManager() public mintAndApproveTokens {
vm.startPrank(user);
rewards = [65, 35];
totalRewards = 100;
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
assertEq(Pot(contest).owner(), conMan); // Pot is owned by the contract
​
vm.prank(player2);
Pot(contest).claimCut(); // remainingRewards = 65
​
uint256 userBalanceBefore = weth.balanceOf(user);
​
vm.warp(91 days);
vm.prank(user);
ContestManager(conMan).closeContest(contest);
​
assertEq(weth.balanceOf(conMan), 6); // cut is in ContestManager
assertEq(weth.balanceOf(user), userBalanceBefore);// admin got nothing
}

The test uses the setup of the existing test/TestMyCut.t.sol (user is the admin, conMan is ContestManager).

Recommended Mitigation

Let the admin withdraw tokens from ContestManager, or forward the cut to the admin right away when closing.

+ import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
contract ContestManager is Ownable {
+ using SafeERC20 for IERC20;
...
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
+ IERC20 token = pot.getToken();
+ uint256 balanceBefore = token.balanceOf(address(this));
pot.closePot();
+ uint256 cut = token.balanceOf(address(this)) - balanceBefore;
+ if (cut > 0) token.safeTransfer(owner(), cut);
}
Updates

Lead Judging Commences

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