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

Manager cuts are sent to `ContestManager`, which has no function to withdraw tokens, so it is locked permanently

theo_mc

Root + Impact

Pot::closePot sends the manager's cut to msg.sender, which is the ContestManager contract (the Pot's owner), not the human owner. ContestManager has no function that can move ERC-20 tokens out, so the manager's cut of every closed contest is permanently locked in ContestManager. This is independent of the divisor bug in closePot: fixing that bug does not change where the cut goes.

Description

The README states that after 90 days the manager takes a cut of the remaining pool. The cut is transferred like this:

// Pot.sol
contract Pot is Ownable(msg.sender) { ... } // owner = whoever deploys the Pot
​
function closePot() external onlyOwner {
...
i_token.transfer(msg.sender, managerCut); // msg.sender = ContestManager
}

The Pot is deployed by ContestManager, and it is also ContestManager that calls closePot:

// ContestManager.sol
function createContest(...) public onlyOwner returns (address) {
Pot pot = new Pot(players, rewards, token, totalRewards); // Pot owner = ContestManager
...
}
​
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
pot.closePot(); // msg.sender inside closePot = ContestManager
}

So the tokens end up on the ContestManager address. Its external functions are:

╭----------------------------------------------------+------------╮
| Method | Identifier |
+=================================================================+
| closeContest(address) | b24eaac8 |
|----------------------------------------------------+------------|
| contestToTotalRewards(address) | 1d01381c |
|----------------------------------------------------+------------|
| contests(uint256) | 9d7ed738 |
|----------------------------------------------------+------------|
| createContest(address[],uint256[],address,uint256) | de9b18c7 |
|----------------------------------------------------+------------|
| fundContest(uint256) | b6bfbe16 |
|----------------------------------------------------+------------|
| getContestRemainingRewards(address) | 2426551e |
|----------------------------------------------------+------------|
| getContestTotalRewards(address) | e172d5ba |
|----------------------------------------------------+------------|
| getContests() | bfaba76a |
|----------------------------------------------------+------------|
| owner() | 8da5cb5b |
|----------------------------------------------------+------------|
| renounceOwnership() | 715018a6 |
|----------------------------------------------------+------------|
| transferOwnership(address) | f2fde38b |
╰----------------------------------------------------+------------╯

None of them transfer ERC-20 tokens out. The inherited Ownable functions (transferOwnership, renounceOwnership) do not touch token balances.

Risk

Likelihood: High

Impact: High

Proof of Concept

Add the following functions inside the existing TestMyCut contract in test/TestMyCut.t.sol. The test uses its existing setup, imports, and mintAndApproveTokens modifier.

address player3 = makeAddr("player3");
​
function _createFundedPot(
uint256 r1,
uint256 r2,
uint256 r3,
uint256 total
) internal returns (Pot) {
address[] memory p = new address[](3);
p[0] = player1;
p[1] = player2;
p[2] = player3;
​
uint256[] memory r = new uint256[](3);
r[0] = r1;
r[1] = r2;
r[2] = r3;
​
vm.startPrank(user);
contest = ContestManager(conMan).createContest(
p,
r,
IERC20(ERC20Mock(weth)),
total
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
return Pot(contest);
}
​
function test_PoC_managerCut_stuckInContestManager() public mintAndApproveTokens {
Pot pot = _createFundedPot(10, 20, 30, 60);
​
vm.prank(player1);
pot.claimCut();
​
uint256 ownerBeforeClose = weth.balanceOf(user);
vm.warp(block.timestamp + 91 days);
vm.prank(user);
ContestManager(conMan).closeContest(address(pot));
​
assertEq(weth.balanceOf(conMan), 5);
assertEq(weth.balanceOf(user), ownerBeforeClose);
}

Run from the repository root:

forge test --mt test_PoC_managerCut_stuckInContestManager -vv

Output:

[PASS] test_PoC_managerCut_stuckInContestManager() (gas: 1193435)
Logs:
User Address: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Contest Manager Address 1: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353
Minting tokens to: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Approved tokens to: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353

Recommended Mitigation

Give the manager a way to receive the cut. Either add a withdrawal function to ContestManager:

+ using SafeERC20 for IERC20; // import SafeERC20 as the repo imports OpenZeppelin
+
+ function withdrawTokens(IERC20 token, address to, uint256 amount) external onlyOwner {
+ token.safeTransfer(to, amount);
+ }

or send the cut directly to a manager address instead of msg.sender in Pot::closePot.

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!