MyCut

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

[M1] `Pot::constructor` Overwrites Rewards for Duplicate Players, Leading to Incorrect Distribution

Summary

The for loop inside the Pot::constructor override the playersToRewards[i_players[i]] with new reward i_rewards[i].So if a player's address appears multiple times, the reward is overwritten rather than accumulated. This results in the player receiving only the reward from the last occurrence of their address in the array, ignoring prior rewards.

Vulnerability Details

Proof of Concept:

  1. Suppose i_players contains [0x123, 0x456, 0x123] and i_rewards contains [100, 200, 300].

  2. The playersToRewards mapping will be updated as follows during construction:

    • For address 0x123 at index 0, reward is set to 300.

    • For address 0x456 at index 1, reward is set to 200.

    • For address 0x123 at index 2, reward is updated to 100.

  3. As a result, the final reward for address 0x123 in playersToRewards will be 100, not 400 (300+100).This leads to incorrect and lower reward distributions.

Proof of Code (PoC):

place the following in the TestMyCut.t.sol::TestMyCut

address player3 = makeAddr("player3");
address player4 = makeAddr("player4");
address player5 = makeAddr("player5");
address[] sixPlayersWithDuplicateOneAddress = [player1, player2, player3, player4, player1, player5];
uint256[] rewardForSixPlayers = [2, 3, 4, 5, 6, 7];
uint256 totalRewardForSixPlayers = 27; // 2+3+4+5+6+7
function test_ConstructorFailsInCorrectlyAssigningReward() public mintAndApproveTokens {
for (uint256 i = 0; i < sixPlayersWithDuplicateOneAddress.length; i++) {
console.log("Player: %s reward: %d", sixPlayersWithDuplicateOneAddress[i], rewardForSixPlayers[i]);
}
/**
* player1 has two occurance in sixPlayersWithDuplicateOneAddress ( at index 0 and 4)
* So it's expected reward should be 2+6 = 8
*/
vm.startPrank(user);
contest = ContestManager(conMan).createContest(sixPlayersWithDuplicateOneAddress, rewardForSixPlayers, IERC20(ERC20Mock(weth)), totalRewardForSixPlayers);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
uint256 expectedRewardForPlayer1 = rewardForSixPlayers[0] + rewardForSixPlayers[4];
uint256 assignedRewardForPlaye1 = Pot(contest).checkCut(player1);
console.log("Expected Reward For Player1: %d", expectedRewardForPlayer1);
console.log("Assigned Reward For Player1: %d", assignedRewardForPlaye1);
assert(assignedRewardForPlaye1 < expectedRewardForPlayer1);
}

Impact

The overall integrity of the reward distribution process is compromised. Players with multiple entries in the i_players[] array will only receive the reward from their last occurrence in the array, leading to incorrect and lower reward distributions.

Tools Used

Manual Review

Recommendations

Recommended Mitigation:
Aggregate the rewards for each player inside the constructor to ensure duplicate addresses accumulate rewards instead of overwriting them.This can be achieved by using the += operator in the loop that assigns rewards to players.

for (uint256 i = 0; i < i_players.length; i++) {
- playersToRewards[i_players[i]] = i_rewards[i];
+ playersToRewards[i_players[i]] += i_rewards[i];
}
Updates

Lead Judging Commences

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

incorrect handling of duplicate addresses

Appeal created

14xSachet Auditor
about 1 year ago
equious Lead Judge
about 1 year ago
14xSachet Auditor
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 handling of duplicate addresses

Support

FAQs

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