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

Incorrect calculation of remaining rewards for distribution among users

arinthal

Summary

The remaining rewards in the Pot are incorrectly calculated for distribution among users.

Vulnerability Details

After the 90-day reward claiming period ends, the admin can close the reward pool using the ContestManager::closeContest function, which triggers the Pot::closePot function. If not all users have claimed their rewards, 10% of the remaining tokens in the pool are transferred to the contract manager, and the rest are distributed among the users who have claimed their rewards.
The error in the contract is that the remaining rewards, after subtracting the portion allocated to the contract manager, are divided by number of all users participating in the contest, but distributed only to those who have claimed their rewards.

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

Impact

When not all eligible users claim their rewards during claim period, the incorrect distribution of the remaining rewards results in a smaller allocation for users who did claim their rewards. Additionally, part of the tokens remain in the contract, preventing their withdrawal.

Proof of Concept

  1. Three users are eligible to claim rewards (300 tokens each)

  2. Two of them claim their rewards during the 90-day period, the third user's reward is not claimed (300 tokens)

  3. After the claim period ends, the admin closes the Pot:

    • the contract manager receives 10% of the remaining rewards (30 tokens)

    • the rest (270 tokens) is divided by number of all participating users

    • rewards are transfered to users who claimed their rewards during the designated period ((300 - 30) / 3 = 90 tokens each), and 90 tokens remain at the contract account

Proof of Code

Add the following code to the TestMyCut.t.sol file within the TestMyCut contract.

function testIncorrectDistributionOfRemainingRewardsAmongClaimers() public mintAndApproveTokens {
address player3 = makeAddr("player3");
players = [player1, player2, player3];
rewards = [300, 300, 300];
totalRewards = 900;
​
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
vm.prank(player1);
Pot(contest).claimCut();
​
vm.prank(player2);
Pot(contest).claimCut();
​
// numOfClaimants - number of users between whom the protocol should divide the remaining rewards
uint256 numOfClaimants = 2;
uint256 player1ClaimedReward = ERC20Mock(weth).balanceOf(player1);
​
vm.warp(block.timestamp + 90 days + 1);
​
uint256 remainingRewards = Pot(contest).getRemainingRewards();
​
vm.prank(user);
ContestManager(conMan).closeContest(contest);
​
uint256 managerCut = ERC20Mock(weth).balanceOf(conMan);
​
// expected player1 balance after rewards distribution
// player1ClaimedReward + (remainingRewards - managerCut)/numOfClaimants
// 300 + (300 - 30) / 2 = 435
uint256 player1ExpectedBalance = player1ClaimedReward + (remainingRewards - managerCut)/numOfClaimants;
uint256 player1CurrentBalance = ERC20Mock(weth).balanceOf(player1);
uint256 remainingFundsAfterClosing = ERC20Mock(weth).balanceOf(contest);
​
assertLt(player1CurrentBalance, player1ExpectedBalance);
assertGt(remainingFundsAfterClosing, 0);
}

Tools Used

  • Manual Review

  • Foundry

Recommended Mitigation

The remaining portion of the reward, unclaimed by users, should be reduced by the portion for the contest manager, and then divided by the number of users who have previously claimed their rewards. To do this, make the following changes to the Pot::closePot function in the Pot.sol file:

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

Lead Judging Commences

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