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

`Pot` constructor overwrites rewards for duplicate player addresses (`=` instead of `+=`)

0xki

Root + Impact

Description

  • The constructor maps each player to their reward so they can later `claimCut`.

  • It assigns with =, so if the same address appears more than once in players, each occurrence **overwrites** the previous value instead of accumulating it. That player can only ever claim the reward from their **last** occurrence; the earlier allocations become unclaimable, and the `Pot` is over-funded relative to claimable rewards — the surplus is stranded.

// src/Pot.sol :: constructor
for (uint256 i = 0; i < i_players.length; i++) {
@> playersToRewards[i_players[i]] = i_rewards[i]; // overwrite on duplicate, not accumulate
}

Risk

Likelihood:

  • Happens whenever the admin-supplied `players` array contains a repeated address — a realistic data-entry mistake, or intentional (a player legitimately earning from multiple sub-events aggregated into one list).

  • There is no validation preventing duplicates.

Impact:

  • Affected players are underpaid — they receive only their last-listed reward.

  • `totalRewards` still reflects the full sum, so the difference is funded into the `Pot` but is never claimable and cannot be recovered — locked funds + unfair distribution.

Proof of Concept

`player1` appears at index 0 (reward 100) and index 2 (reward 300). Expected claimable = 400; actual stored = 300. After `player1` and `player2` claim, 100 tokens are orphaned in the `Pot`.

function test_H03_duplicateOverwrite() public mintAndApproveTokens {
address[] memory p = new address[](3);
p[0] = player1; p[1] = player2; p[2] = player1; // player1 duplicated
uint256[] memory r = new uint256[](3);
r[0] = 100; r[1] = 200; r[2] = 300;
​
vm.startPrank(user);
contest = ContestManager(conMan).createContest(p, r, IERC20(ERC20Mock(weth)), 600);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
assertEq(Pot(contest).checkCut(player1), 300); // expected 100+300=400, stored only 300
​
vm.prank(player1); Pot(contest).claimCut();
vm.prank(player2); Pot(contest).claimCut();
assertEq(ERC20Mock(weth).balanceOf(contest), 100); // 100 orphaned by the overwrite
}

Result: `[PASS]` — `checkCut(player1) == 300`, and 100 tokens are left stranded.

Recommended Mitigation

Accumulate rewards for duplicate addresses:


Additionally consider validating `players.length == rewards.length` and that `Σ rewards == totalRewards` in the constructor.

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

ai-first-flight-judge Lead Judge about 2 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

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

## Description 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` ```Solidity 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. ## 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. ```diff for (uint256 i = 0; i < i_players.length; i++) { - playersToRewards[i_players[i]] = i_rewards[i]; + playersToRewards[i_players[i]] += i_rewards[i]; } ```

Support

FAQs

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

Give us feedback!