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

Loss of funds due to incorrect ownership management

0xiceman

Summary

After closing the contest, the part of the reward that has not been distributed in the 90 days, should be distributed to the manager (10% cut), and to those who claimed their rewards on time. However, there is an issue with incorrect ownership management, leading to the loss of unclaimed rewards and improper distribution of the manager's cut.

Vulnerability Details

The root cause of this vulnerability is that the Pot contract is owned by the ContestManager contract instead of the EOA (Externally Owned Account) that creates and manages the contests. This leads to the fact that the manager's cut (10% of unclaimed rewards) is sent to the ContestManager contract instead of the actual manager (EOA).

The critical lines could be found here:

  1. Creating a new contest

  2. Setting owner to the msg.sender

Proof of Concept:

Place the following test into the ./test/TestMyCut.t.sol:

function testInvalidOwnerOfThePot() public mintAndApproveTokens {
vm.startPrank(user);
rewards = [500, 500];
totalRewards = 1000;
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
​
uint256 userBalanceBefore = ERC20Mock(weth).balanceOf(user);
​
vm.warp(91 days);
​
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
​
uint256 userBalanceAfter = ERC20Mock(weth).balanceOf(user);
​
assert(ERC20Mock(weth).balanceOf(conMan) == (1000-500)/10);
assert(userBalanceBefore == userBalanceAfter);
}

Impact

Loss of Funds: The actual manager (EOA) does not receive their 10% cut of unclaimed rewards, as it's sent to the ContestManager contract instead:

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);
​
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

Locked Funds: The manager's cut becomes locked in the ContestManager contract with no way to retrieve it, effectively losing these funds.

Tools Used

Manual review / Foundry tests

Recommendations

Transfer Pot Ownership: Modify the createContest function in ContestManager to transfer ownership of the newly created Pot to the owner of the ContestManager (the EOA):

function createContest(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards)
public
onlyOwner
returns (address)
{
Pot pot = new Pot(players, rewards, token, totalRewards);
+ pot.transferOwnership(owner()); // Transfer ownership to the ContestManager owner
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}

Modify Access Control: Replace the onlyOwner modifier in the closePot function of the Pot contract with a new onlyContestManager modifier.

modifier onlyContestManager() {
require(msg.sender == address(ContestManager), "Only ContestManager can call this function");
_;
}
​
function closePot() external onlyContestManager {
// ... existing code ...
}

Add onlyOwner to ContestManager's closePot: Ensure that only the owner of ContestManager can close a contest.

function closeContest(address contest) public onlyOwner {
_closeContest(contest);
}

Transfer tokens to the owner

- i_token.transfer(msg.sender, managerCut);
+ i_token.transfer(owner(), managerCut);

By implementing these changes, the contracts will ensure proper ownership management, correct distribution of the manager's cut, and fair distribution of remaining rewards among timely claimants.

Updates

Lead Judging Commences

equious Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Owner's cut is stuck in ContestManager

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.