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

Duplicate player addresses silently overwrite rewards, creating permanent insolvency

diegotrigueros

Root + Impact

Description

  • The constructor loop sets playersToRewards[players[i]] = rewards[i] without checking for duplicate addresses. If the same address appears twice, the second entry overwrites the first, but remainingRewards is set to totalRewards which includes both reward amounts.

  • The first reward is counted in remainingRewards but can never be claimed, permanently desynchronizing accounting.

for (uint256 i = 0; i < i_players.length; i++) {
@> playersToRewards[i_players[i]] = i_rewards[i]; // overwrites if duplicate address
}

Risk

Likelihood:

  • The owner creates a contest where a player appears in the list more than once (common mistake in airdrop/reward lists, or intentional if the same player wins multiple categories).

  • No duplicate check exists on-chain; the second mapping write silently overwrites the first.

Impact:

  • The duplicated player can only claim the last assigned reward value, not the sum of all their entries.

  • remainingRewards includes all reward entries (including the overwritten one), creating a permanent surplus that can never be claimed or properly distributed.

  • This surplus gets partially distributed in closePot but with the wrong divisor (see H-01), compounding the fund locking issue.

Proof of Concept

The following test shows that when the same address 0x1 appears twice in the players array with different reward amounts, the second entry overwrites the first. The player can only claim the second (smaller) amount, and the difference is permanently locked in the contract.

function testDuplicatePlayerOverwrite() public {
address[] memory players = new address[](3);
uint256[] memory rewards = new uint256[](3);
players[0] = address(0x1); rewards[0] = 50 ether; // first entry for 0x1
players[1] = address(0x1); rewards[1] = 10 ether; // DUPLICATE — overwrites to 10
players[2] = address(0x2); rewards[2] = 40 ether;
// totalRewards = 100, but claimable = 10 + 40 = 50
​
Pot pot = new Pot(players, rewards, token, 100 ether);
token.transfer(address(pot), 100 ether);
​
vm.prank(address(0x1));
pot.claimCut(); // claims 10, not 50
​
vm.prank(address(0x2));
pot.claimCut(); // claims 40
​
// 50 tokens permanently stuck in contract
assertEq(token.balanceOf(address(pot)), 50 ether);
}

Recommended Mitigation

Add a duplicate check in the constructor loop. Before writing to the mapping, verify that the player address does not already have an assigned reward. This prevents silent overwrites and ensures each player appears exactly once.

​
for (uint256 i = 0; i < i_players.length; i++) {
+ require(playersToRewards[i_players[i]] == 0, "Duplicate player");
playersToRewards[i_players[i]] = i_rewards[i];
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 2 days 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!