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

Pot constructor overwrites reward mapping for duplicate addresses, permanently locking overwritten amounts

virgilbb

Root + Impact

Description

  • Pot::constructor is supposed to record each player's individual reward so they can later call claimCut() and withdraw exactly that amount. totalRewards is set to the sum of all reward entries so the pot holds enough tokens to pay everyone.

  • The constructor assigns rewards with = instead of +=. When the same address appears more than once in players[], each iteration overwrites the previous value. Only the last assignment survives; all earlier reward amounts for that address are counted in remainingRewards but have no claimant and cannot be withdrawn.

// Pot.sol constructor
for (uint256 i = 0; i < i_players.length; i++) {
// @> = overwrites prior entries for the same address
playersToRewards[i_players[i]] = i_rewards[i];
}

Risk

Likelihood:

  • ContestManager::createContest accepts the players[] array directly from the owner with no deduplication check, so a clerical error (copy-paste, off-by-one in tooling) silently introduces duplicates.

  • No revert or warning is produced at creation time, so the mistake is only discoverable after players attempt to claim.

Impact:

  • For a player appearing twice with rewards [R1, R2], only R2 is claimable. R1 is counted in remainingRewards but owned by no one — it stays locked in the pot until closePot, where it is diluted across all players and incorrectly paid to past claimants.

  • The net result is incorrect distribution and irrecoverable stuck funds for every pot that contains a duplicated address.

Proof of Concept

Create a pot where Alice appears twice with rewards 300e18 and 100e18. The second entry overwrites the first; Alice can claim only 100e18, and 300e18 is permanently locked.

function testDuplicatePlayerLocksReward() public mintAndApproveTokens {
address alice = makeAddr("alice");
​
address[] memory dupPlayers = new address[](2);
dupPlayers[0] = alice;
dupPlayers[1] = alice;
​
uint256[] memory dupRewards = new uint256[](2);
dupRewards[0] = 300e18;
dupRewards[1] = 100e18;
​
vm.startPrank(user);
address pot = ContestManager(conMan).createContest(
dupPlayers, dupRewards, IERC20(weth), 400e18
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
// Alice can only claim 100e18 — the 300e18 entry was overwritten
vm.prank(alice);
Pot(pot).claimCut();
​
assertEq(weth.balanceOf(alice), 100e18);
// 300e18 is permanently stuck with no owner
assertEq(Pot(pot).getRemainingRewards(), 300e18);
}

getRemainingRewards() returns 300e18 after Alice's claim, confirming the overwritten amount is trapped.

Recommended Mitigation

Use += when building the reward mapping so duplicate entries accumulate rather than overwrite. Optionally, add an explicit duplicate check to revert early on bad input.

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

For stricter validation, revert on any duplicate address at construction time:

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

Lead Judging Commences

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