H-03: Duplicate Players in Constructor Overwrite Rewards - Silent Fund Loss
Root + Impact
Root Cause
In Pot.sol lines 43-47, the constructor uses simple assignment (=) when populating the playersToRewards mapping, causing duplicate player entries to overwrite previous rewards instead of accumulating:
Impact
-
Silent fund loss - first reward entry permanently lost
-
Malicious owner can exploit by creating duplicate entries
-
Accidental duplicates (frontend bugs) cause unexpected losses
-
No recovery possible - overwritten data gone forever
Description
The Pot constructor uses simple assignment (=) when populating the playersToRewards mapping, causing duplicate player entries to overwrite previous rewards instead of accumulating them.
Risk
Likelihood:
-
Silent fund loss - first reward entry permanently lost
-
Malicious owner can exploit by creating duplicate entries
-
Accidental duplicates (frontend bugs) cause unexpected losses
-
No recovery possible - overwritten data gone forever
Impact:
Impact Assessment
| Dimension | Assessment |
|---|---|
| Fund Loss | Direct - first entry tokens lost |
| Attack Vector | Malicious owner can create duplicate entries to steal |
| User Error | Accidental duplicates cause silent fund loss |
| Recoverability | Impossible - overwritten data gone |
Severity Justification: High - Silent fund loss with no recovery. Can be exploited by malicious owner or occur accidentally.
Proof of Concept
Result: ✅ PASSED - player1 receives 500 instead of 600
Execution Command:
Recommended Mitigation
Three Recomendation mitigation option:
Option 1: Accumulate Rewards (Recommended)
-
Changes: Replace the assignment operator (=) with an addition operator (+=) in the player reward initialization loop.
-
Benefit: Ensures that if a player address is listed multiple times, their rewards are safely summed up instead of being silently overwritten and lost.
Option 2: Reject Duplicates (Defense in Depth)
-
Changes: Add a validation check (require) inside the constructor loop to ensure each player's reward slot has not already been initialized (== 0).
-
Benefit: Acts as a secondary safeguard to explicitly block contract deployment if duplicate player addresses are detected.
Option 3: Validate in ContestManager.createContest()
-
Changes: Implement pre-deployment validation logic within the factory contract (ContestManager) to check for duplicate entries and zero addresses.
-
Benefit: Prevents invalid configurations at the source level before any child contract is deployed.
Best Practice: Combine Option 1 (accumulate in Pot) + Option 3 (validate in ContestManager)
Lead Judging Commences
[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]; } ```
Rewards breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Judging
Submissions are being reviewed by our AI judge. Results will be available in a few minutes.
View all submissionsRewards Distribution
The contest is complete and the rewards are being distributed.