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

closePot Transfers 10% Manager Fee to ContestManager Contract Which Has No Withdraw Function — Fee Permanently Locked

bamgboseoluwole83

Root + Impact

Description

  • Describe the normal behavior in one or more sentences

  • Explain the specific issue or problem in one or more sentences

# [H-#] `closePot` Transfers 10% Manager Fee to `ContestManager` Contract Which Has No Withdraw Function — Fee Permanently Locked
​
## Summary
When a contest is closed via `ContestManager.closeContest()`, the underlying contract executes `Pot.closePot()`. Inside `Pot.closePot()`, the 10% manager fee cut is transferred directly to `msg.sender`. Because the caller is the `ContestManager` contract itself, the fee tokens are permanently routed to its address. Since `ContestManager` lacks any withdrawal mechanism or sweep function, these protocol fees become permanently frozen and unrecoverable.
​
## Vulnerability Details
In `Pot.sol`, the closure function distributes the designated manager percentage as follows:
```solidity
// @audit msg.sender here is the ContestManager contract when called via closeContest
i_token.transfer(msg.sender, managerCut);
```
​
When managing multiple distributions, administrative routines are funneled through `ContestManager.sol` using a central call:
```solidity
function closeContest(address contest) external onlyOwner {
Pot(contest).closePot();
}
```
​
During this execution frame, the EVM records the caller (`msg.sender`) of `Pot.closePot()` as the `ContestManager` deployment address, rather than the transaction initiator (the protocol owner). The 10% manager cut transfers successfully into the `ContestManager` balance. However, because `ContestManager` is a rigid routing contract with no asset sweep or `withdraw` functionalities, any tokens received on its balance sheet are permanently trapped.
​
## Impact
**High.** The primary revenue generation loop for the protocol manager is broken. Every completed contest permanently traps 10% of its remaining prize pool allocations inside an unresolvable contract state, resulting in a total loss of protocol fee yield.
​
## Proof of Concept
Add the following test case to your suite to confirm the fee entrapment:
​
```solidity
function test_H01_ManagerCutStuckInContestManager() public {
// 1. Simulate a standard participant claim to leave a remainder pool
vm.prank(player1);
Pot(contest).claimCut();
// 2. Fast forward past the mandatory lock period window
vm.warp(block.timestamp + 91 days);
// 3. Owner closes the contest through the central manager entry point
vm.prank(owner);
conMan.closeContest(contest);
// 4. Assert that the manager cut was pushed into ContestManager and is unrecoverable
uint256 trappedFees = weth.balanceOf(address(conMan));
assertGt(trappedFees, 0);
console.log("Trapped manager fee tokens:", trappedFees);
}
```
​
## Tools Used
* Manual Code Review
​
## Recommendations
Implement an asset withdrawal mechanism inside `ContestManager.sol` to allow administrative recovery of accumulated fee tokens:
​
```solidity
// @audit-issue Add to ContestManager.sol to rescue trapped protocol fees
function withdrawFees(IERC20 token, address recipient) external onlyOwner {
uint256 balance = token.balanceOf(address(this));
require(balance > 0, "No fee balance available");
token.transfer(recipient, balance);
}
```
​
Alternatively, refactor `Pot.closePot()` to bypass `msg.sender` routing, instead explicitly sending the fee to a pre-configured, immutable treasury wallet or an administrative destination address passed during compilation.
// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • Reason 1 // Describe WHEN this will occur (avoid using "if" statements)

  • Reason 2

Impact:

  • Impact 1

  • Impact 2

Proof of Concept

​

Recommended Mitigation

- remove this code
+ add this code
Updates

Lead Judging Commences

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