MyCut

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

Reward Distribution Inconsistency

Summary

The current implementation of the reward distribution function contains a critical inconsistency that can lead to excess tokens being trapped in the contract.

Code in Question

uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}

Description

The bug arises from an inconsistency between how the claimantCut is calculated and how it's distributed:

  1. claimantCut is calculated using i_players.length, which represents all players.

  2. The distribution loop uses claimants.length, which represents only the players who claimed in time.

This inconsistency can lead to excess tokens being left in the contract when i_players.length > claimants.length.

Impact

  1. Excess Tokens: When there are fewer claimants than total players, a portion of the rewards will remain undistributed in the contract.

  2. Trapped Funds: The excess rewards become trapped in the contract as there is no independent function to transfer these tokens out.

  3. Manager Unable to Retrieve: The contract manager cannot access or redistribute these excess tokens.

Root Cause

The root cause is the mismatch between the calculation of claimantCut and its distribution. The calculation should use claimants.length instead of i_players.length to accurately distribute the remaining pool to those who claimed in time.

Steps to Reproduce

  1. Set up a scenario where i_players.length > claimants.length.

  2. Execute the reward distribution function.

  3. Observe that some tokens remain in the contract after distribution.

Recommended Fix

Update the claimantCut calculation to use claimants.length:

- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
+ _totalClaimans = claimants.length;
+ uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
for (uint256 i = 0; i < _totalClaimans; i++) {
_transferReward(claimants[i], claimantCut);
}

Additionally, consider implementing a function that allows the contract manager to withdraw any excess tokens that might accumulate due to rounding errors or edge cases.

Additional Recommendations

  1. Add checks to ensure claimants.length is not zero before performing the division.

  2. Implement a mechanism to handle any remaining dust (small amounts left due to division rounding).

  3. Consider using a pull pattern for reward distribution to mitigate potential issues with failed transfers.

Updates

Lead Judging Commences

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.