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

The Pot::closePot function uses an incorrect formula to calculate claimantCut on closing the contest Pot leads to claimants getting less claimant cut than they should receive

aeff

Relevant GitHub Links

https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L49

Summary

The closePot() function in the Pot.sol contract miscalculates the claimantCut that should be sent to all claimants when the contest manager closes the Pot.

Vulnerability Details

The closePot() function in the Pot.sol contract uses incorrect formula to calculate the claimantCut on closing the pot by incorrectly dividing the remaining rewards (after subtracting the managerCut) by the total number of participating players i_players.length as the denominator instead of dividing by the actual number of claimants claimants.length. This error results in a diluted claimant cut, causing the rewards to be unfairly distributed among those who claimed, and part of the remaining rewards will be left unused on the Pot ERC20 token balance.

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

Example Pot Scenario:

Number of participating players: 4

Rewards for each player: 50

Total Rewards: 200

Two players have claimed his cut. When the admin calls Pot::closePot function, the calculations will be as follows as per current implementation:

remainingRewards = 100

managerCut = remainingRewards / managerCutPercent = 100/10 = 10

claimantCut = (100-10)/4 = 22 (Rounded down from 22.5)

Each claimant will get 22 token as claimant cut, and 46 rewards will be left on the Pot ERC20 token balance due to the incorrect claimant cut calculation.

Pot rewards balance after claimant cut distribution = 46

The correct claimant cut calculation should be (100-10)/claimants.length = (100-10)/2 which should results to 45. As a result each claimant gets 45 tokens cut of the remaining rewards on closing the Pot, and each claimant should have a total token balance of (50 + 45) = 95 tokens in our example. Also the Pot ERC20 token balance should be 0 on closing the Pot if the correct formula is used

Impact

1- Unfair distribution of claimant cut: Claimants will get less claimant cut on closing put than they should get leads to unfair distribution of remaining claimant cut among all claimants.

2- Part of the remaining reward tokens are left on the Pot balance: Part of the remainingReward will be still in the pot contract ERC20 token balance since a less claimant cut has been calculated and distributed than it should have been calculated.

Proof of Concept##

To demonstrate the issue, add the following test in TestMyCut.t.sol, and execute the following command:

forge test --mt testCloseContenstClaimantCut -vv

function testCloseContenstClaimantCut() public mintAndApproveTokens {
address player01 = makeAddr("player1");
address player02 = makeAddr("player2");
address player03 = makeAddr("player3");
address player04 = makeAddr("player4");
address[] memory totalPlayers = new address[](4);
totalPlayers[0] = player01;
totalPlayers[1] = player02;
totalPlayers[2] = player03;
totalPlayers[3] = player04;
uint256[] memory playerRewards = new uint256[](4);
for (uint256 i = 0; i < playerRewards.length; i++) {
playerRewards[i] = 50;
}
uint256 totalPotRewards = 200;
​
// Create contest Pot
console.log("Contest Manger creates the Pot");
vm.startPrank(user);
contest = ContestManager(conMan).createContest(
totalPlayers,
playerRewards,
IERC20(ERC20Mock(weth)),
totalPotRewards
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
// First player claims his reward (50)
console.log("Player01 claims his reward (50)");
vm.startPrank(player01);
Pot(contest).claimCut();
vm.stopPrank();
​
// Second player claims his reward (50)
console.log("Player02 claims his reward (50)");
vm.startPrank(player02);
Pot(contest).claimCut();
vm.stopPrank();
​
// Balance of all players before closing the Pot
uint256 balance = ERC20Mock(weth).balanceOf(player01);
console.log("Player01 balance before closing bot: ", balance);
balance = ERC20Mock(weth).balanceOf(player02);
console.log("Player02 balance before closing bot: ", balance);
balance = ERC20Mock(weth).balanceOf(player03);
console.log("Player03 balance before closing bot: ", balance);
balance = ERC20Mock(weth).balanceOf(player04);
console.log("Player04 balance before closing bot: ", balance);
balance = ERC20Mock(weth).balanceOf(contest);
console.log(
"Pot Balance after two claimants and before closing bot: ",
balance
);
​
vm.warp(91 days);
​
// Admin closes the Pot
console.log("#########################################");
console.log("Contest Manger closes the Pot");
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
​
// Balance of all players after closing the Pot
balance = ERC20Mock(weth).balanceOf(player01);
console.log("Player01 balance before closing bot: ", balance);
assertNotEq(balance, 95);
balance = ERC20Mock(weth).balanceOf(player02);
console.log("Player02 balance before closing bot: ", balance);
assertNotEq(balance, 95);
balance = ERC20Mock(weth).balanceOf(player03);
console.log("Player03 balance before closing bot: ", balance);
balance = ERC20Mock(weth).balanceOf(player04);
console.log("Player04 balance before closing bot: ", balance);
uint256 potBalance = ERC20Mock(weth).balanceOf(contest);
console.log(
"Pot balance after after closing the pbot (2 claimants): ",
potBalance
);
assert(potBalance > 0);
}

Tools Used

Manual code review and Foundry

Recommendations

It is recommended to fix the formula inside the Pot::closePot function used to calculate the claimantCut when closing the Pot by dividing by the total number of claimants claimants.length as the denominator rather than by the total number of participating players i_players.length. This adjustment ensures that claimants receive the correct share of the remaining rewards when the contest manager closes the Pot and avoids leaving unused ERC20 tokens in the balance of the Pot contract.

+ uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
Updates

Lead Judging Commences

equious Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect distribution in closePot()

Appeal created

aeff Submitter
about 1 year ago
equious Lead Judge
about 1 year ago
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.