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

Incorrect Reward Distribution in `Pot::closePot` (Logical Error + Potential Financial Loss)

andir16

Description

The Pot::closePot function contains a logical error that results in incorrect distribution of remaining rewards when the pot is closed. Specifically, when the pot is closed after 90 days, the contract attempts to distribute the remaining rewards among the claimants that claimed in time, but it incorrectly calculates the amount to be distributed. The contract uses the formula (remainingRewards - managerCut) / i_players.length, which fails to properly account for players that have not claimed, resulting in a situation where part of the remaining tokens is left in the contract, contrary to the intended behavior. The remaining pot should be distributed among players that successfully claimed and therefore, the remaining cut should also be calculated with those players and not the total amount of players.

Contract description:

MyCut is a contest rewards distribution protocol which allows the set up and management of multiple rewards distributions, allowing 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!

Impact

This issue can lead to significant financial losses for participants, as the undistributed tokens remain locked in the contract, potentially forever. This contradicts the intended functionality, where all unclaimed rewards should be distributed among the claimants. The funds remaining in the contract are effectively lost to all parties involved.

Proof of Concepts

Consider the following scenario:

  • The total reward pool is 1000 tokens.

  • Two players (Player 1 and Player 2) are entitled to equal rewards (500 tokens each).

  • Player 1 claims their reward, leaving 500 tokens in the contract.

  • After 91 days, the contract owner calls Pot::closePot, expecting the remaining tokens to be distributed.

  • The contract takes a 10% manager cut of the remaining 500 tokens (50 tokens), leaving 450 tokens for distribution.

  • The players cut it then calculated with 2 players instead of 1 (as player2 is not eligible since he didn't claim) and therefore divides the remainder (450 tokes) by 2.

  • Due to the flawed logic, Player 1 only receives 225 tokens instead of the full 450 tokens, leaving 225 tokens in the contract.

The test case (add this to TestMyCut.t.sol):

function testRewardsFullyDistributed() 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();
​
// Player 1 claims their cut
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
​
// Warp time to allow closing the contest
vm.warp(91 days);
​
// Owner closes the contest
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
​
// Player 1 claimed 500, leaving 500 in the pot
// Constest is closed and 10% (50) are given to the Contest Manager
// Remaining 450. According to the logic player 1 should get all 450
// Because player2 did not claim. But player 1 only gets 225 so he will have 725 tokens
​
assertEq(ERC20Mock(weth).balanceOf(player1), 725);
​
// Check that the contract's balance still holds 225 tokens
uint256 contractBalanceAfterClose = ERC20Mock(weth).balanceOf(contest);
assertEq(contractBalanceAfterClose, 225);
}

The test passes showing that player1 only received 725 and that there are 225 remaining in the contract.

Recommended Mitigation

To fix this issue, adjust the reward distribution logic in Pot::closePot to ensure that all remaining tokens (after the manager's cut) are fully distributed to the claimants. Specifically, calculate the claimant's share by dividing the remaining tokens by the total number of claimants instead of the total number of players:

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

This adjustment ensures that all remaining rewards are correctly distributed among the actual claimants, preventing any leftover tokens in the contract.

Updates

Lead Judging Commences

equious Lead Judge 10 months 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.