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

RC-01: Manager's Cut Permanently Locked in ContestManager + Residual Funds Trapped in Pot Contract

r7al38

Root + Impact

Two separate fund locking vulnerabilities exist in the closePot function: the manager's 10% cut gets sent to the msg.sender (the ContestManager contract) without any withdrawal mechanism, and the remaining rewards distribution calculation leaves excess tokens permanently stuck in the Pot contract.

Description

  • Normal behavior: After 90 days, the contract owner should be able to close the pot, receive their 10% management fee, and distribute the remaining rewards proportionally among all players who haven't claimed yet.

  • The specific issues: First, when closePot is called by the ContestManager contract (the owner), the manager's cut is transferred directly to the ContestManager address. Since the ContestManager contract I looked at doesn't have any functions to withdraw ERC20 tokens, these funds are stuck there forever with no way to retrieve them.

  • Second, the distribution calculation is completely broken. The code calculates claimantCut by dividing the remaining amount (after manager cut) by the total number of players (i_players.length), but then only sends this amount to the actual claimants (claimants array). If some players never claimed their rewards (which is exactly why we're closing the pot), the difference between what should be distributed and what actually gets sent remains locked in the Pot contract indefinitely.

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); // @audit - sends to ContestManager with no withdraw function
​
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
@> for (uint256 i = 0; i < claimants.length; i++) { // @audit - only sends to claimants, not all players
_transferReward(claimants[i], claimantCut);
}
// @audit - remaining funds after this loop are never handled
}
}

Risk

Likelihood:High

  • The manager's cut will be locked 100% of the time when the owner is a contract (like ContestManager) that doesn't implement token withdrawal functions

  • Excess funds will remain in the Pot contract every single time closePot is called and not all players have claimed their rewards

Impact:High

  • The protocol's manager fees become permanently inaccessible, resulting in direct financial loss for the team/owner

  • Unclaimed player rewards that should be redistributed end up stuck in the contract instead of going to active participants

  • Multiple pots closing over time will accumulate trapped funds, leading to significant value leakage from the protocol

Proof of Concept

I wrote a Foundry test that demonstrates both vulnerabilities in action. The test shows that after closePot is called:

  1. The ContestManager contract holds 50 tokens with no way to transfer them out

  2. The Pot contract still holds 150 tokens that should have been distributed

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import "forge-std/Test.sol";
import "../src/Pot.sol";
import {ERC20} from "openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
​
contract ERC20Mock is ERC20 {
constructor() ERC20("Mock", "MCK") {
_mint(msg.sender, 1_000_000 * 10 ** decimals());
}
​
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
}
​
// ContestManager fake contract - does not contain any token withdrawal function
contract ContestManagerMock {
}
​
contract PotTest is Test {
ERC20Mock token;
Pot pot;
ContestManagerMock contestManager;
address[] players;
uint256[] rewards;
uint256 totalRewards = 1000 * 10 ** 18; // 1000
​
address alice = address(0x1);
address bob = address(0x2);
address charlie = address(0x3);
​
function setUp() public {
token = new ERC20Mock();
// Mint tokens for this contract (test) to fund the Pot contract
token.mint(address(this), totalRewards);
​
// Player profiles and rewards
players = [alice, bob, charlie];
rewards = [200e18, 300e18, 500e18]; // Total = 1000e18
​
contestManager = new ContestManagerMock();
​
//Publish the Pot contract with contestManager as the owner (using vm.prank)
vm.startPrank(address(contestManager));
pot = new Pot(players, rewards, token, totalRewards);
vm.stopPrank();
​
//Converting tokens from test to contract (Pot)
token.transfer(address(pot), totalRewards);
}
​
function testLockedManagerCutAndResidualFunds() public {
// Players Alice and Bob are demanding their rewards
vm.prank(alice);
pot.claimCut();
​
vm.prank(bob);
pot.claimCut();
​
// Skip 90 days
vm.warp(block.timestamp + 90 days);
​
//The manager (contestManager) calls closePot
vm.prank(address(contestManager));
pot.closePot();
​
// Now let's calculate the expected results:
// Remaining amount = totalRewards - (Alice + Bob's claims) = 1000 - (200 + 300) = 500
// Manager's share = 10% of the remaining amount = 500 * 10% = 50
// Amount distributed to claimants = (500 - 50) / Total number of pl ayers (3) = 450 / 3 = 150 per player
// But the ring only sends 150 to the claimants (Alice, Bob) = 300 each = 300
// So:
// - 50 was sent to contestManager
// - 300 was sent to Alice and Bob
// - Pot remains in the contract = 500 - 350 = 150 (it should have gone to Charlie, but he's not in the claimants)
​
// Verify that the manager's stake is stuck in ContestManager
uint256 managerBalance = token.balanceOf(address(contestManager));
assertEq(managerBalance, 50e18, "Manager cut should be locked in ContestManager");
​
//Check that the remaining funds are stuck in the pot.
uint256 potBalance = token.balanceOf(address(pot));
assertEq(potBalance, 150e18, "Residual funds should be locked in Pot");
​
// Verify that Alice and Bob have received their original rewards + bonus share
assertEq(token.balanceOf(alice), 200e18 + 150e18, "Alice balance incorrect");
assertEq(token.balanceOf(bob), 300e18 + 150e18, "Bob balance incorrect");
// Charlie got nothing because he didn't claim anything.
assertEq(token.balanceOf(charlie), 0, "Charlie balance should be 0");
​
// Trying to prove that funds cannot be withdrawn from ContestManager (there is no withdrawal function)
// We can only confirm that no one can call a transfer from the token using the contestManager address
// Because it's a codeless contract, it can't execute any function.
// Trying to call a non-existent withdrawal function (will fail)
// But the test works because we're not trying to do that; we're just observing that the balances are stuck.
// To confirm, we can try using vm.prank to simulate contestManager attempting to transfer tokens
// But since contestManager has no code, any call from its address will be vm.prank to a codeless address
// This can't work because there's no code to execute the function. So we just check the balances.
}
}
​

Recommended Mitigation

Two fixes are needed here. First, we need to ensure the manager's cut goes to an address that can actually receive and manage funds - either send directly to the owner's EOA or implement a pull-based withdrawal pattern. Second, the distribution logic needs to properly divide the remaining rewards among the actual claimants, not all players.

The key changes:

  1. Send manager's cut directly to owner() instead of msg.sender to ensure it goes to a known address that can handle funds

  2. Calculate claimant cut based on actual claimants.length so all remaining funds are distributed

  3. Handle any remainder from integer division to prevent micro-amounts from getting stuck

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);
+ // Send directly to owner's EOA or implement a withdrawal pattern
+ i_token.transfer(owner(), managerCut);
​
- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
+ uint256 amountForClaimants = remainingRewards - managerCut;
+ uint256 claimantCut = amountForClaimants / claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
+
+ // Handle any remainder from division
+ uint256 distributed = claimantCut * claimants.length;
+ if (distributed < amountForClaimants) {
+ uint256 remainder = amountForClaimants - distributed;
+ // Option 1: Send remainder to manager as extra fee
+ i_token.transfer(owner(), remainder);
+ // Option 2: Burn the tokens
+ // i_token.transfer(0x000000000000000000000000000000000000dEaD, remainder);
+ }
}
}
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!