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

Unfair distribution of the remaining rewards in `Pot::closePot()` function

0xiceman

Summary

The Pot contract allows authorized claimants 90 days to claim before the manager takes a cut of the remaining pool and the remainder is distributed equally to those who claimed in time. However, due to incorrect implementation of the Pot::closePot function, funds could remain locked in the contract whenever the contest is closed and there is at least one player who didn't claim. Additionally, the distribution of remaining funds is unfair to those who did claim.

Vulnerability Details

The root cause of the problem is the line marked bellow:

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);
}
}
}

According to the documentation, the remaining rewards (if any) should be distributed as follows:

  1. 10% to the manager

  2. Remainder equally among claimants

However, this implementation is dividing claimantCut by i_players.length instead of claimants.length. This leads to an unfair distribution where:

  • If some players don't claim, their share is effectively lost.

  • The total amount distributed to claimants is less than it should be.

  • Funds may remain locked in the contract.

Proof of Concept:

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

function testUnfairDistributionOfRemainingFunds() public mintAndApproveTokens {
vm.startPrank(user);
//add more players to demontstrate this
rewards = [220, 230, 500, 50];
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();
​
vm.warp(91 days);
​
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
//dividing by 4 since 4 players, even tough only 1 of them claimed
//(780-78)/4 = 175.5
assert(ERC20Mock(weth).balanceOf(player1) == (rewards[0] + 175));
//220 -> claim
//175 -> distribution of remaining
//78 -> 10% cut
assert(ERC20Mock(weth).balanceOf(contest) == totalRewards - 220 - 175 - 78);
}

Impact

Due to incorrect implementation of this function:

  1. Funds will remain locked in the Pot contract in any scenario where there is at least one player that didn't claim. (There is no ability to withdraw the locked funds.)

  2. Claimants receive less than their fair share of the remaining funds.

Tools Used

Manual code review / Foundry tests

Recommendations

  1. Consider the scenario when there are 0 claimants, and divide with claimants.length instead of i_players.length:

function closePot() external onlyContestManager {
//modifier is set to the onlyContestManager as suggested in some of my other reports
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
if (claimants.length == 0) {
// If no one claimed, transfer all remaining funds to the owner
//transfer to the owner() as suggested in some of my other reports
i_token.transfer(owner(), remainingRewards);
} else {
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(owner(), managerCut);
uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
remainingRewards = 0;
}
}
Updates

Lead Judging Commences

equious Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect distribution in closePot()

Support

FAQs

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