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

Incorrect calculation of the `claimantCut` value in the `Pot::claimCut` function leads to funds being locked.

elprofesor

Summary

If any player fails to claim their reward within 90 days after the contest is created, the user will close the contest using the ContestManager::closeContest function. During this process, the unclaimed reward will be distributed such that 10% goes to the user, and the rest is evenly split among the players who have claimed their reward. The issue is that, instead of dividing the remaining amount by the number of claimants, the current implementation divides it by the total number of players. This results in claimants receiving less than they should, with the excess funds being unjustly withheld and locked in the contract.

Vulnerability Details

ContestManager::closeContest

ContestManager::_closeContest

Pot::closePot

The vulnerability is located in the Pot::closePot function, which is called from the ContestManager::_closeContest function, which is in turn called by the ContestManager::closeContest function.

In the Pot::closePot function, the claimantCut is calculated by dividing the remaining rewards (after the manager's cut) by the total number of players (i_players.length), instead of dividing it by the number of claimants (claimants.length).

Place the following test in the test/TestMyCut.s.sol file:

function testBadCalucationOf90DaysCut() public mintAndApproveTokens {
uint256 userBalanceBefore = weth.balanceOf(user); // 1000_000000000000000000
uint256 player1BalanceBefore = weth.balanceOf(player1); // 0
uint256 player2BalanceBefore = weth.balanceOf(player2); // 0
​
vm.startPrank(user);
rewards = [200 ether, 200 ether];
totalRewards = 400 ether;
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
vm.prank(player1);
Pot(contest).claimCut();
​
vm.warp(91 days);
vm.prank(user);
ContestManager(conMan).closeContest(contest);
​
uint256 userBalanceAfter = weth.balanceOf(user); // 600_000000000000000000
uint256 player1BalanceAfter = weth.balanceOf(player1); // 290_000000000000000000
uint256 player2BalanceAfter = weth.balanceOf(player2); // 0
​
uint256 player1ExpectedBalanceAfterCloseContest = 200 ether + 180 ether; // This is the expected balance
assert(player1BalanceAfter != player1ExpectedBalanceAfterCloseContest);
​
uint256 player1RealBalanceAfterCloseContest = 200 ether + 90 ether; // This is the actual balance under the current implementation
assert(player1BalanceAfter == player1RealBalanceAfterCloseContest);
}

Impact

When closing the contest, the claimants will receive less than they deserve, and the portion that is taken from them will be locked in the contract.

Tools Used

Manual Code Review, Foundry Test

Recommended Mitigation

Line that should be changed

Instead of dividing by i_players.length in the Pot::closePot function, it should be divided by claimants.length.

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

By making this change, the claimants will receive the correct proportion of the remaining rewards when the contest is closed.

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.