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

`Pot::closePot` does not properly distribute remaining rewards to claimants

NHristov

Summary

The Pot::closePot function does not properly distribute the remaining rewards to the claimants. The calculation for the claimant's cut uses the length of i_players instead of claimants, which can lead to incorrect distribution. Additionally, there is no check to ensure that there is at least one claimant before distributing the rewards.

Vulnerability Details

The Pot::closePot function in the Pot contract calculates the claimant's cut using the length of i_players instead of claimants. This can result in incorrect distribution of the remaining rewards. Furthermore, if there are no claimants, the remaining rewards should be transferred to the manager instead of being divided among zero claimants.

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

Proof Of Concept

After the pot has ended (90 days have passed), the owner can call the closePot function. If there are no claimants, the remaining rewards will be incorrectly divided among zero claimants, leading to incorrect distribution.

Place the following test into TestMyCut.t.sol

function testIncorrectClaimantDistribution() public mintAndApproveTokens {
totalRewards = 400;
vm.startPrank(user);
rewards = [200, 200];
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();
​
// Check if remaining rewards are incorrectly distributed
uint256 managerBalance = ERC20Mock(weth).balanceOf(conMan);
uint256 expectedPlayer1Balance = totalRewards - managerBalance;
assert(ERC20Mock(weth).balanceOf(player1) != expectedPlayer1Balance);
}

Impact

If the Pot::closePot function is called, the remaining rewards will be incorrectly distributed among the claimants. This can lead to:

  • Incorrect reward distribution to claimants.

  • Potentially leaving tokens locked in the contract

Tools Used

  • Solidity compiler

  • Manual code review

  • Foundry

Recommendations

To mitigate this vulnerability, update the Pot::closePot function to use the length of claimants instead of i_players for calculating the claimant's cut. Additionally, add a check to ensure that there is at least one claimant before distributing the rewards. If there are no claimants, a good idea is to transfer the remaining rewards to the manager. Here is an updated version of the Pot contract with the recommended changes:

function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
if (claimants.length == 0){
i_token.transfer(msg.sender, remainingRewards);
}
else{
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
​
uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}
}
Updates

Lead Judging Commences

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

Incorrect distribution in closePot()

Incorrect handling of zero claimant edge case

Support

FAQs

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