## Summary
The use of `msg.sender` in `Pot.sol:55` will cause permanent loss of 10% management fee for the admin as the `closePot()` function transfers the manager cut to the `ContestManager` contract (the caller) instead of the actual owner, and `ContestManager` has no withdrawal mechanism.
## Root Cause
In `Pot.sol:55` the manager cut is sent to the wrong recipient:
```solidity
function closePot() external onlyOwner {
// ...
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut); // ✗ BUG: msg.sender = ContestManager, NOT admin
// ...
}
}
```
When `closeContest()` is called on `ContestManager`, it internally calls `pot.closePot()`. At this point:
- `msg.sender` = `ContestManager` contract address
- `owner()` = `ContestManager` contract address (set in constructor)
But the actual human admin who should receive the fee is the owner of `ContestManager`, not `ContestManager` itself.
**Critical:** `ContestManager.sol` has NO withdrawal function to recover stuck tokens.
## Internal Pre-conditions
1. Any contest needs to be closed via `ContestManager.closeContest()`
2. This is the ONLY way to close contests (required behavior)
## External Pre-conditions
None - this is a pure protocol design bug.
## Attack Path
This is not an active attack but a vulnerability path during normal protocol operation:
1. Admin creates a contest with 1000 tokens total rewards
2. After 90 days, admin calls `ContestManager.closeContest(potAddress)`
3. `ContestManager` internally calls `pot.closePot()`
4. `closePot()` calculates `managerCut = 1000 / 10 = 100 tokens`
5. Tokens are transferred to `msg.sender` which is `ContestManager` contract
6. Admin receives 0 tokens
7. 100 tokens are stuck forever in `ContestManager` (no withdraw function)
## Impact
The admin suffers permanent loss of 100% of all management fees:
| Scenario | Contests | Manager Cut per Contest | Total Stuck |
|----------|----------|------------------------|-------------|
| Single | 1 | 100 tokens | **100 tokens** |
| Multiple | 5 | 100 tokens | **500 tokens** |
| Protocol Lifetime | 100 | 100 tokens | **10,000 tokens** |
**Quantified Impact:**
- For every 1000 ETH in rewards, admin loses 100 ETH (10%) permanently
- This compounds over the lifetime of the protocol
- **No recovery mechanism exists**
## PoC
```solidity
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
import {Test, console} from "forge-std/Test.sol";
import {ContestManager} from "../../src/ContestManager.sol";
import {Pot} from "../../src/Pot.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
/**
* @title ERC20Mock - Simple mock ERC20 for testing
*/
contract ERC20Mock is IERC20 {
string public name;
string public symbol;
uint8 public decimals = 18;
uint256 public totalSupply;
mapping(address => uint256) public balanceOf;
mapping(address => mapping(address => uint256)) public allowance;
constructor(string memory _name, string memory _symbol) {
name = _name;
symbol = _symbol;
}
function mint(address to, uint256 amount) external {
balanceOf[to] += amount;
totalSupply += amount;
}
function transfer(address to, uint256 amount) external returns (bool) {
balanceOf[msg.sender] -= amount;
balanceOf[to] += amount;
emit Transfer(msg.sender, to, amount);
return true;
}
function approve(address spender, uint256 amount) external returns (bool) {
allowance[msg.sender][spender] = amount;
emit Approval(msg.sender, spender, amount);
return true;
}
function transferFrom(address from, address to, uint256 amount) external returns (bool) {
allowance[from][msg.sender] -= amount;
balanceOf[from] -= amount;
balanceOf[to] += amount;
emit Transfer(from, to, amount);
return true;
}
}
/**
* @title ManagerCutWrongRecipientPoC
* @notice PoC demonstrating that manager cut goes to ContestManager contract, not the admin
*
* ## Vulnerability Summary
* In Pot.sol:55, the manager cut is sent to `msg.sender` which is the ContestManager
* contract (the one calling closeContest), NOT the actual owner/admin.
*
* ContestManager has NO withdrawal function, so these funds are permanently stuck.
*
* ## Root Cause
* ```solidity
* i_token.transfer(msg.sender, managerCut); // msg.sender = ContestManager, NOT admin
* ```
*
* Should be:
* ```solidity
* i_token.transfer(owner(), managerCut); // owner() = the actual admin
* ```
*/
contract ManagerCutWrongRecipientPoC is Test {
ContestManager public contestManager;
ERC20Mock public token;
address public admin = makeAddr("admin");
address public player1 = makeAddr("player1");
address public player2 = makeAddr("player2");
address[] public players;
uint256[] public rewards;
uint256 public constant TOTAL_REWARDS = 1000 ether;
function setUp() public {
players = new address[](2);
players[0] = player1;
players[1] = player2;
rewards = new uint256[](2);
rewards[0] = 500 ether;
rewards[1] = 500 ether;
vm.startPrank(admin);
contestManager = new ContestManager();
token = new ERC20Mock("Test Token", "TEST");
token.mint(admin, TOTAL_REWARDS);
token.approve(address(contestManager), TOTAL_REWARDS);
vm.stopPrank();
}
/**
* @notice Test demonstrating manager cut goes to wrong recipient
*/
function test_ManagerCutGoesToContestManager_NotAdmin() public {
console.log("=== Manager Cut Wrong Recipient Vulnerability PoC ===");
console.log("");
// Step 1: Admin creates and funds contest
vm.startPrank(admin);
address potAddress = contestManager.createContest(
players,
rewards,
IERC20(address(token)),
TOTAL_REWARDS
);
contestManager.fundContest(0);
vm.stopPrank();
console.log("[SETUP]");
console.log(" Admin address: %s", admin);
console.log(" ContestManager address: %s", address(contestManager));
console.log(" Pot address: %s", potAddress);
console.log(" Total Rewards: %s tokens", TOTAL_REWARDS / 1e18);
console.log("");
// Step 2: No one claims, fast forward 91 days
vm.warp(block.timestamp + 91 days);
// Record balances before
uint256 adminBalBefore = token.balanceOf(admin);
uint256 contestManagerBalBefore = token.balanceOf(address(contestManager));
console.log("[BEFORE CLOSE]");
console.log(" Admin balance: %s tokens", adminBalBefore / 1e18);
console.log(" ContestManager balance: %s tokens", contestManagerBalBefore / 1e18);
console.log("");
// Step 3: Admin closes the contest
vm.prank(admin);
contestManager.closeContest(potAddress);
// Step 4: Check where the manager cut went
uint256 adminBalAfter = token.balanceOf(admin);
uint256 contestManagerBalAfter = token.balanceOf(address(contestManager));
uint256 adminReceived = adminBalAfter - adminBalBefore;
uint256 contestManagerReceived = contestManagerBalAfter - contestManagerBalBefore;
// Expected: Manager should get 10% = 100 tokens
uint256 expectedManagerCut = TOTAL_REWARDS / 10;
console.log("[AFTER CLOSE - BUG DEMONSTRATED]");
console.log(" Expected manager cut: %s tokens", expectedManagerCut / 1e18);
console.log(" Admin received: %s tokens (SHOULD be 100!)", adminReceived / 1e18);
console.log(" ContestManager received: %s tokens (BUG!)", contestManagerReceived / 1e18);
console.log("");
// ContestManager has NO withdrawal function!
console.log("[IMPACT]");
console.log(" Manager cut is STUCK in ContestManager contract");
console.log(" ContestManager has NO withdraw function");
console.log(" Funds are PERMANENTLY UNRECOVERABLE");
console.log("");
// Assertions
assertEq(adminReceived, 0, "Admin should receive NOTHING - this is the bug");
assertEq(contestManagerReceived, expectedManagerCut, "ContestManager gets the cut instead");
console.log("=== PoC SUCCESSFUL: Manager's %s tokens stuck forever ===", contestManagerReceived / 1e18);
}
/**
* @notice Verify ContestManager has no withdrawal mechanism
*/
function test_ContestManagerHasNoWithdrawFunction() public {
console.log("=== Verifying ContestManager Has No Withdraw ===");
console.log("");
// Send some tokens directly to ContestManager to simulate stuck funds
vm.prank(admin);
token.mint(address(contestManager), 100 ether);
uint256 stuckFunds = token.balanceOf(address(contestManager));
console.log("ContestManager balance: %s tokens", stuckFunds / 1e18);
// List all functions ContestManager has:
console.log("");
console.log("ContestManager available functions:");
console.log(" - createContest() [cannot withdraw]");
console.log(" - fundContest() [cannot withdraw]");
console.log(" - closeContest() [cannot withdraw]");
console.log(" - getContests() [view only]");
console.log(" - getContestTotalRewards() [view only]");
console.log(" - getContestRemainingRewards() [view only]");
console.log("");
console.log(">>> NO WITHDRAW FUNCTION EXISTS <<<");
console.log(">>> FUNDS ARE PERMANENTLY LOCKED <<<");
// Funds remain stuck
assertEq(token.balanceOf(address(contestManager)), 100 ether, "Funds stuck forever");
}
/**
* @notice Multiple contests compound the damage
*/
function test_MultipleContests_AccumulatingLoss() public {
console.log("=== Multiple Contests = Accumulating Loss ===");
console.log("");
// Fund admin with more tokens
vm.prank(admin);
token.mint(admin, 4000 ether);
vm.prank(admin);
token.approve(address(contestManager), 5000 ether);
// Create 5 contests
vm.startPrank(admin);
for (uint i = 0; i < 5; i++) {
contestManager.createContest(
players,
rewards,
IERC20(address(token)),
TOTAL_REWARDS
);
contestManager.fundContest(i);
}
vm.stopPrank();
console.log("[SETUP] Created and funded 5 contests");
console.log(" Each contest: %s tokens", TOTAL_REWARDS / 1e18);
console.log(" Total deposited: %s tokens", (5 * TOTAL_REWARDS) / 1e18);
console.log("");
// Fast forward and close all
vm.warp(block.timestamp + 91 days);
address[] memory allContests = contestManager.getContests();
vm.startPrank(admin);
for (uint i = 0; i < allContests.length; i++) {
contestManager.closeContest(allContests[i]);
}
vm.stopPrank();
uint256 stuckInContestManager = token.balanceOf(address(contestManager));
console.log("[RESULT]");
console.log(" Contests closed: 5");
console.log(" Manager cut per contest: 100 tokens");
console.log(" Total stuck in ContestManager: %s tokens", stuckInContestManager / 1e18);
console.log("");
console.log("=== Admin NEVER receives their management fees! ===");
// 5 contests * 100 tokens each = 500 tokens stuck
assertEq(stuckInContestManager, 500 ether, "All manager cuts stuck");
}
}
```
### Files Included
- `ManagerCutWrongRecipientPoC.t.sol` - Main PoC test file
### Run Command
```bash
forge test --match-contract ManagerCutWrongRecipientPoC -vvv
```
### Expected Output
```
[PASS] test_ContestManagerHasNoWithdrawFunction() (gas: 59500)
Logs:
=== Verifying ContestManager Has No Withdraw ===
ContestManager balance: 100 tokens
ContestManager available functions:
- createContest() [cannot withdraw]
- fundContest() [cannot withdraw]
- closeContest() [cannot withdraw]
- getContests() [view only]
- getContestTotalRewards() [view only]
- getContestRemainingRewards() [view only]
>>> NO WITHDRAW FUNCTION EXISTS <<<
>>> FUNDS ARE PERMANENTLY LOCKED <<<
[PASS] test_ManagerCutGoesToContestManager_NotAdmin() (gas: 1000261)
Logs:
=== Manager Cut Wrong Recipient Vulnerability PoC ===
[SETUP]
Admin address: 0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF
ContestManager address: 0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395
Pot address: 0x9113f1FCD23283cD8f52f20EA11369b860b947F7
Total Rewards: 1000 tokens
[AFTER CLOSE - BUG DEMONSTRATED]
Expected manager cut: 100 tokens
Admin received: 0 tokens (SHOULD be 100!)
ContestManager received: 100 tokens (BUG!)
[IMPACT]
Manager cut is STUCK in ContestManager contract
ContestManager has NO withdraw function
Funds are PERMANENTLY UNRECOVERABLE
=== PoC SUCCESSFUL: Manager's 100 tokens stuck forever ===
[PASS] test_MultipleContests_AccumulatingLoss() (gas: 4577712)
Logs:
=== Multiple Contests = Accumulating Loss ===
[SETUP] Created and funded 5 contests
Each contest: 1000 tokens
Total deposited: 5000 tokens
[RESULT]
Contests closed: 5
Manager cut per contest: 100 tokens
Total stuck in ContestManager: 500 tokens
=== Admin NEVER receives their management fees! ===
Suite result: ok. 3 passed; 0 failed; 0 skipped
```
## Mitigation
**Option 1:** Forward the manager cut to the actual admin via a dedicated admin address:
```diff
// In Pot.sol
+address private managerAddress; // Set in constructor
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);
+ i_token.transfer(managerAddress, managerCut); // Use stored admin address
// ...
}
}
```
**Option 2:** Add a withdrawal function to `ContestManager`:
```diff
// In ContestManager.sol
+function withdrawStuckTokens(IERC20 token, uint256 amount) external onlyOwner {
+ token.transfer(msg.sender, amount);
+}
```
**Recommended:** Option 1 is preferred as it fixes the root cause rather than adding a workaround.
