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

`Pot::closePot` has erraneous math , causing claimants to get less rewards and some money to be left in the contract

SoulReaper

Summary

Pot::closePot has erroneous math , causing claimants to get less rewards and some money to be left in the contract

Vulnerability Details

The docs clearly state that when pot has to be closed and funds are left, manager takes his cut and remaining balance has to be distributed among those who claimed in time. Look at the following line from closePot function:

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

claimantCut is being found out by divinding the remaining balance ((remainingRewards - managerCut)) by the total number of players (i_players.length) . This is contradictory to the docs , as if the remaining balance has to distributed equally among the claimants , then remaining balance should be divided by the total number of claimants , which is claimants.length

Due to this wrong calculation , claimants get less rewards than they should and also some funds are left in the contract

Impact

Due to this wrong calculation , claimants get less rewards than they should and also some funds are left in the contract

Proof of Concepts

  1. Owner creates and funds the pool

  2. Player 1 claims

  3. Deadline passes

  4. Owner calls closePot

  5. Owner gets his 10% (no bug here)

  6. There was only 1 claimer , and he should have gotten all the remaining balance of 450 , but since this got divided by 2 (num of players) , he only got 225

  7. The contract , which should've been empty , still contains some money.

Paste this into TestMyCut.t.sol

function test_ClosePotHasWrongMath() 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();
​
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
​
uint256 claimantBalanceBefore = ERC20Mock(weth).balanceOf(player1);
uint256 ownerBalanceBefore = ERC20Mock(weth).balanceOf(conMan);
​
vm.warp(91 days);
​
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
​
uint256 claimantBalanceAfter = ERC20Mock(weth).balanceOf(player1);
uint256 ownerBalanceAfter = ERC20Mock(weth).balanceOf(conMan);
​
assert(ownerBalanceAfter - ownerBalanceBefore == 50); // no bug here
​
// assert(claimantBalanceAfter > claimantBalanceBefore);
// assert(claimantBalanceAfter - claimantBalanceBefore == 450); --> fails due to bug
assert(claimantBalanceAfter - claimantBalanceBefore == 225);
assert(ERC20Mock(weth).balanceOf(contest) == 225 ); // has non zero balance left
}

Tools Used

Manual Review , Foundry Tests

Recommendations

Change the way claimantCut is calculated:

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 9 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.