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

Pot::closePot pays the 10% manager cut to msg.sender, which is always the ContestManager contract (the Pot's owner); ContestManager has no function to move tokens out, so every manager fee is locked forever

friedsocks233

Manager cut sent to the ContestManager contract + all protocol fee revenue permanently locked

Description

  • The README says that after the 90-day claim period "the manager takes a cut of the remaining pool". The manager is the Owner/Admin who creates, funds and closes pots, so the 10% cut should reach that admin.

  • Pot is Ownable(msg.sender) and ContestManager::createContest deploys it with new Pot(...), so the owner of every Pot is the ContestManager contract. closePot is onlyOwner, so it can only be reached through ContestManager::closeContest, and it transfers managerCut to msg.sender. That is the ContestManager contract, not the admin.

  • ContestManager's only token-moving call is transferFrom(msg.sender, pot, totalRewards) in fundContest, which spends the admin's own balance. It has no withdraw or sweep function and no fallback, and it never calls Pot.transferOwnership. The pinned OpenZeppelin 5.0.2 Ownable adds only owner, renounceOwnership and transferOwnership.

  • So the fee can never leave ContestManager. Even if "manager" is read as the ContestManager contract itself, the cut is unusable, because that contract has no way to spend it.

// src/Pot.sol
@> 7:contract Pot is Ownable(msg.sender) {
// ...
49: function closePot() external onlyOwner {
50: if (block.timestamp - i_deployedAt < 90 days) {
51: revert Pot__StillOpenForClaim();
52: }
53: if (remainingRewards > 0) {
54: uint256 managerCut = remainingRewards / managerCutPercent;
@> 55: i_token.transfer(msg.sender, managerCut);
// src/ContestManager.sol
16: function createContest(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards)
17: public
18: onlyOwner
19: returns (address)
20: {
21: // Create a new Pot contract
@> 22: Pot pot = new Pot(players, rewards, token, totalRewards);
23: contests.push(address(pot));
24: contestToTotalRewards[address(pot)] = totalRewards;
25: return address(pot);
26: }
// ...
53: function closeContest(address contest) public onlyOwner {
54: _closeContest(contest);
55: }
56:
57: function _closeContest(address contest) internal {
58: Pot pot = Pot(contest);
59: pot.closePot();
60: }

Risk

Likelihood:

  • This happens on every closeContest call where remainingRewards > 0, which means whenever at least one player did not claim. That is the normal case the close step exists for.

  • No attacker and no admin mistake is involved. The trusted admin following the intended flow triggers it on every pot, for the lifetime of the deployment.

Impact:

  • All of the protocol's fee revenue, 10% of every unclaimed pool across all pots, is frozen in ContestManager, and the admin receives none of it. In the PoC the admin's balance stays at 0 while ContestManager gains 50 per close, and three pots lock 150.

  • The loss is permanent. No code path, inherited or otherwise, moves ContestManager's token balance. Player funds are not affected.

Proof of Concept

The test deploys ContestManager and the repo's test/ERC20Mock.sol and uses the real createContest, fundContest and closeContest flow. All three tests pass:

  • test_managerCutLockedInContestManager:

    • It asserts pot.owner() == address(conMan), and that a direct pot.closePot() call from the admin reverts (onlyOwner).

    • p1 claims 500 and the admin closes. ContestManager's balance goes from 0 to 50, and the admin's balance is unchanged at 0.

    • The helper _assertNoExit then shows the fee cannot leave. Calls to withdraw(), withdraw(address), withdraw(address,uint256), sweep(address), transfer(address,uint256) and rescueTokens(address,address,uint256) on ContestManager all fail. ContestManager has granted no allowance. fundContest cannot recycle the fee, because it spends only the admin's balance.

  • test_feesAccumulateAcrossPots: three closed pots lock 150 in ContestManager, and the admin receives none of it.

  • test_repeatedCloseFeedsTheSinkUntilPotIsDrained: with zero claimants, ten repeated closes move the whole 1000 into ContestManager, and the next close reverts. This test depends on the separate non-terminal closePot issue and is included only to bound the amount.

Save the file as test/PoC_MC-02.t.sol and run from the repository root:

FOUNDRY_FFI=false forge test --match-path test/PoC_MC-02.t.sol -vv
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import {Test, console} from "lib/forge-std/src/Test.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {ContestManager} from "../src/ContestManager.sol";
import {Pot} from "../src/Pot.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
​
/// MC-02: the 10% manager cut is paid to the ContestManager contract (the Pot's owner),
/// which has no function able to move its own token balance, so every fee is frozen.
contract PoC_MC02 is Test {
address admin = makeAddr("admin");
address p1 = makeAddr("p1");
address p2 = makeAddr("p2");
address p3 = makeAddr("p3");
address p4 = makeAddr("p4");
​
ContestManager conMan;
ERC20Mock token;
​
function setUp() public {
vm.startPrank(admin);
conMan = new ContestManager();
token = new ERC20Mock("TKN", "TKN", admin, 0);
token.mint(admin, 1000);
token.approve(address(conMan), type(uint256).max);
vm.stopPrank();
}
​
function _createAndFund(address[] memory players, uint256[] memory rewards, uint256 total)
internal
returns (Pot pot)
{
vm.startPrank(admin);
address potAddr = conMan.createContest(players, rewards, IERC20(address(token)), total);
conMan.fundContest(conMan.getContests().length - 1);
vm.stopPrank();
pot = Pot(potAddr);
}
​
/// Main path from the finding: 1000 pot, [p1,p2] at 500 each, only p1 claims.
function test_managerCutLockedInContestManager() public {
address[] memory players = new address[](2);
players[0] = p1;
players[1] = p2;
uint256[] memory rewards = new uint256[](2);
rewards[0] = 500;
rewards[1] = 500;
Pot pot = _createAndFund(players, rewards, 1000);
​
// Root cause: the Pot's owner is the ContestManager contract, not the admin EOA.
assertEq(pot.owner(), address(conMan), "pot owner is ContestManager");
assertTrue(pot.owner() != admin, "admin is not the pot owner");
assertEq(token.balanceOf(admin), 0, "admin spent all tokens funding the pot");
​
// Baseline: a direct call by the admin cannot close the pot (onlyOwner).
vm.warp(block.timestamp + 91 days);
vm.prank(admin);
vm.expectRevert();
pot.closePot();
​
vm.warp(block.timestamp - 91 days);
vm.prank(p1);
pot.claimCut();
assertEq(pot.getRemainingRewards(), 500, "remainingRewards = 500");
​
vm.warp(block.timestamp + 91 days);
uint256 adminBefore = token.balanceOf(admin);
uint256 cmBefore = token.balanceOf(address(conMan));
vm.prank(admin);
conMan.closeContest(address(pot));
​
uint256 adminAfter = token.balanceOf(admin);
uint256 cmAfter = token.balanceOf(address(conMan));
console.log("admin balance before/after close:", adminBefore, adminAfter);
console.log("ContestManager balance before/after close:", cmBefore, cmAfter);
​
// Impact: the 10% cut (50) lands in the ContestManager contract; the admin gets nothing.
assertEq(adminAfter, adminBefore, "admin receives no manager cut");
assertEq(cmAfter - cmBefore, 50, "50 tokens (10% of 500) sent to ContestManager");
​
_assertNoExit(50);
}
​
/// Repeated over many pots, the frozen balance only grows.
function test_feesAccumulateAcrossPots() public {
vm.prank(admin);
token.mint(admin, 2000); // admin now holds 3000
​
address[] memory players = new address[](2);
players[0] = p1;
players[1] = p2;
uint256[] memory rewards = new uint256[](2);
rewards[0] = 500;
rewards[1] = 500;
​
uint256 expectedLocked;
for (uint256 k = 0; k < 3; k++) {
Pot pot = _createAndFund(players, rewards, 1000);
vm.prank(p1);
pot.claimCut();
vm.warp(block.timestamp + 91 days);
vm.prank(admin);
conMan.closeContest(address(pot));
expectedLocked += 50;
}
console.log("ContestManager balance after 3 closed pots:", token.balanceOf(address(conMan)));
assertEq(token.balanceOf(address(conMan)), expectedLocked, "150 tokens locked after 3 pots");
assertEq(token.balanceOf(admin), 0, "admin received none of the 150 fee tokens");
_assertNoExit(expectedLocked);
}
​
/// Amplification limb (depends on the separate repeated-close bug): zero claimants,
/// every extra closeContest moves another 10% into the sink, bounded by the Pot's balance.
function test_repeatedCloseFeedsTheSinkUntilPotIsDrained() public {
address[] memory players = new address[](2);
players[0] = p1;
players[1] = p2;
uint256[] memory rewards = new uint256[](2);
rewards[0] = 500;
rewards[1] = 500;
Pot pot = _createAndFund(players, rewards, 1000);
​
vm.warp(block.timestamp + 91 days);
vm.startPrank(admin);
for (uint256 k = 0; k < 10; k++) {
conMan.closeContest(address(pot));
}
// Pot is now empty; the next close reverts (limb is bounded by the pot balance).
vm.expectRevert();
conMan.closeContest(address(pot));
vm.stopPrank();
​
console.log("ContestManager balance after 10 closes:", token.balanceOf(address(conMan)));
console.log("Pot balance after 10 closes:", token.balanceOf(address(pot)));
assertEq(token.balanceOf(address(conMan)), 1000, "entire pot moved into ContestManager");
assertEq(token.balanceOf(address(pot)), 0, "pot drained");
_assertNoExit(1000);
}
​
/// Shows that nothing reachable can move ContestManager's token balance out.
function _assertNoExit(uint256 locked) internal {
// 1. No withdraw/sweep-style function exists (calls fall through with no fallback -> revert).
bytes[] memory probes = new bytes[](6);
probes[0] = abi.encodeWithSignature("withdraw()");
probes[1] = abi.encodeWithSignature("withdraw(address)", address(token));
probes[2] = abi.encodeWithSignature("withdraw(address,uint256)", address(token), locked);
probes[3] = abi.encodeWithSignature("sweep(address)", address(token));
probes[4] = abi.encodeWithSignature("transfer(address,uint256)", admin, locked);
probes[5] = abi.encodeWithSignature("rescueTokens(address,address,uint256)", address(token), admin, locked);
vm.startPrank(admin);
for (uint256 i = 0; i < probes.length; i++) {
(bool ok,) = address(conMan).call(probes[i]);
assertFalse(ok, "no withdraw-style function on ContestManager");
}
​
// 2. ContestManager never granted an allowance on its own balance.
assertEq(token.allowance(address(conMan), admin), 0, "no allowance to admin");
​
// 3. fundContest cannot recycle the locked fees: it only spends the admin's own balance.
address[] memory pl = new address[](1);
pl[0] = p3;
uint256[] memory rw = new uint256[](1);
rw[0] = locked;
conMan.createContest(pl, rw, IERC20(address(token)), locked);
uint256 idx = conMan.getContests().length - 1;
uint256 adminBal = token.balanceOf(admin);
if (adminBal < locked) {
vm.expectRevert(ContestManager.ContestManager__InsufficientFunds.selector);
conMan.fundContest(idx);
}
vm.stopPrank();
​
// 4. The fee is still in ContestManager and the admin is no richer.
assertEq(token.balanceOf(address(conMan)), locked, "fees remain frozen in ContestManager");
assertEq(token.balanceOf(admin), adminBal, "admin balance unchanged");
}
}

Recommended Mitigation

Forward the amount that closePot delivered on to the admin in the same call. An alternative is an onlyOwner withdraw(IERC20 token) on ContestManager.

--- a/src/ContestManager.sol
+++ b/src/ContestManager.sol
@@ -56,7 +56,11 @@
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
+ IERC20 token = pot.getToken();
+ uint256 balanceBefore = token.balanceOf(address(this));
pot.closePot();
+ // forward the manager cut that closePot sent to this contract on to the admin
+ token.transfer(owner(), token.balanceOf(address(this)) - balanceBefore);
}
}
Updates

Lead Judging Commences

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