MyCut

Summary

In the closePot() function, a manager can claim the rewards multiple times if the number of total players (i_players.length) exceeds the number of claimants (claimants.length) and the condition remainingRewards / managerCutPercent <= i_token.balanceOf(address(this)) is true. This loophole can lead to the manager repeatedly taking cuts from the pot, draining the remaining rewards unfairly.

Vulnerability Details

The vulnerability arises from two factors:

1. i_players > claimants: When the number of total players exceeds the number of claimants, the reward calculation for claimants is based on the total number of players (i_players.length) instead of the actual claimants (claimants.length). https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L57

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

   }

   }

   }

2. remainingRewards / managerCutPercent <= i_token.balanceOf(address(this)): This condition enables the manager to successfully claim again if the remaining rewards, after dividing by the managerCutPercent, are less than or equal to the token balance of the contract. In this case, the manager can repeatedly call closePot() and drain the remaining rewards over multiple calls.

The combination of these two issues allows the manager to repeatedly claim rewards, even though it should have been fairly distributed among claimants.

Example Scenario Illustrating the Bug:

1. Initial Setup:

   • remainingRewards = 1000

   • i_players.length = 10 (total number of players)

   • claimants.length = 5 (number of players who claimed)

   • managerCutPercent = 10 (10% manager cut)

2. First Call to closePot():

   • The manager takes 10% of remainingRewards:
     managerCut = 1000 / 10 = 100

   • Claimant cut is incorrectly calculated based on i_players.length:
     claimantCut = (remainingRewards - managerCut) / i_players.length = (1000 - 100) / 10 = 90

   • Each of the 5 claimants receives 90 tokens.

   • Remaining balance of the contract after the first call:
     900 - (90 * 5) = 900 - 450 = 450

3. Second Call to closePot():

   • If remainingRewards / managerCutPercent <= i_token.balanceOf(address(this)), the manager can claim again: managerCut = 450 / 10 = 45

   • Remaining rewards after the second manager cut:
     remainingRewards = 450 - 45 = 405

   • Each of the 5 claimants again receives less:
     claimantCut = (405 - managerCut) / 10 = 405 / 10 = 40.5

   • Remaining rewards after the second call:
     405 - (40.5 * 5) = 405 - 202.5 = 202.5

4. Third Call to closePot():

   • The manager continues to claim as long as the condition holds true, eventually depleting the remaining rewards.

Impact

1. Multiple Claims by the Manager: The manager can repeatedly claim cuts from the pot, even though rewards should have been fairly distributed among the claimants after the first call.

2. Unfair Distribution: Due to the incorrect calculation of rewards for claimants, the rewards are split among a larger number of players (i_players.length), leaving the actual claimants with much less than they are entitled to.

3. Depletion of Remaining Rewards: The pot's remaining rewards are gradually depleted, potentially leaving little to nothing for the actual claimants after multiple manager claims.

Steps to Reproduce the Bug:

1. Step 1: Deploy the Pot contract with 10 players and a reward pool of 1000 tokens.

2. Step 2: Allow only 5 players to claim their rewards.

3. Step 3: Call closePot() multiple times as the manager, ensuring that the condition remainingRewards / managerCutPercent <= i_token.balanceOf(address(this)) holds after each call.

4. Expected Outcome: The pot should be closed after one manager claim, with the remaining rewards distributed among the claimants.

5. Actual Outcome: The manager can claim multiple times, draining the remaining rewards.

Tools Used

Manual Review

Recommendations

To fix the issue, the following changes should be made:

• Use claimants.length for Distribution: Replace the current reward distribution logic, which uses i_players.length, with claimants.length to ensure only those who have claimed receive rewards.

• Prevent Multiple Manager Claims: Add a boolean state variable (e.g., isPotClosed) or a modifier (e.g., potNotClosed) to track whether the pot has already been closed. This will prevent multiple calls from the manager. Once the pot is closed, the variable should be set to true, blocking any further attempts to call the closePot() function.