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

Unfair Reward Distribution Due to Integer Division in Claimant Cut Calculation

iamthesvn

Summary

There's a problem in how the smart contract calculates rewards for claimants. The current method can lead to unfair distribution of rewards and might leave some tokens stuck in the contract.

Relevant code

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

Vulnerability Details

The issue is in this line of code:
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;

This calculation has several problems:

  1. If (remainingRewards - managerCut) is less than i_players.length, claimants get nothing.

  2. It always rounds down, which can be unfair, especially with small amounts.

  3. Some tokens might get stuck in the contract because of rounding down.

Impact

This problem affects how fairly rewards are given out. Some claimants might get less than they should, or nothing at all, even when there are rewards to give. This isn't just about losing money - it could make people lose trust in the system. If users think the system is unfair, they might not want to use it.

POC

Here's a POC that demonstrates the vulnerability using the context and structure of the Pot contract:

contract VulnerablePot is Pot {
function demonstrateVulnerability() public view returns (uint256[3] memory) {
uint256[3] memory results;
uint256 sampleRemainingRewards = 100 ether;
uint256 sampleManagerCut = 10 ether;
​
for (uint256 i = 0; i < 3; i++) {
uint256 playerCount = 30 + i * 10; // 30, 40, 50 players
uint256 claimantCut = (sampleRemainingRewards - sampleManagerCut) / playerCount;
results[i] = claimantCut;
}
​
return results; // Will return [3 ether, 2 ether, 1 ether]
}
}

This POC extends the Pot contract and demonstrates that:

  1. With 30 players, each gets 3 ether (1 ether remains undistributed)

  2. With 40 players, each gets 2 ether (10 ether remains undistributed)

  3. With 50 players, each gets 1 ether (40 ether remains undistributed)

In each case, the undistributed amount is effectively stuck in the contract due to integer division. This closely mirrors the actual behaviour of the closePot function in the original contract, where claimantCut is calculated similarly.

Tools Used

Manual review

Recommendations

Can implement a scaled calculation approach to address this vulnerability.

Updates

Lead Judging Commences

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

Incorrect distribution in closePot()

Dusty Pot

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.