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

Duplicate Players in createContest() Overwrite Reward Mapping Causing Tokens to Be Permanently Stuck

mkmustapha1

Root + Impact

Description

  • The Pot constructor is designed to map each player address to their individual reward amount so they can claim it later via claimCut().

  • The constructor loops over the players array and assigns playersToRewards[players[i]] = rewards[i] with no duplicate address check. If the same address appears more than once, the second assignment silently overwrites the first. The full totalRewards amount was funded for both entries, but only the last reward value is claimable — the first reward amount is permanently stuck in the contract. Additionally, i_players.length still counts the duplicate, compounding the wrong-divisor bug in closePot().

// Pot.sol constructor
constructor(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards) {
i_players = players;
i_rewards = rewards;
i_token = token;
i_totalRewards = totalRewards;
remainingRewards = totalRewards;
i_deployedAt = block.timestamp;
​
for (uint256 i = 0; i < i_players.length; i++) {
// @> No duplicate check on players[i]
// @> If player1 appears at index 0 AND index 2:
// @> First: playersToRewards[player1] = 300 ← silently overwritten
// @> Second: playersToRewards[player1] = 100 ← only this is claimable
// @> But totalRewards was funded as 300 + 100 = 400
// @> 300 tokens are permanently stuck — player1 can only claim 100
playersToRewards[i_players[i]] = i_rewards[i];
}
}

Risk

Likelihood: High

  • createContest() in ContestManager has no duplicate address validation — it passes the raw arrays directly to the Pot constructor with zero sanitization

  • Any admin error or malicious input with a repeated address immediately triggers this with no on-chain protection

Impact: High

  • Tokens equal to all but the last duplicate reward entry are permanently locked — claimCut() can only recover the final overwritten value

  • The stuck tokens can never be recovered — there is no sweep, rescue, or secondary claim function in either contract

  • The duplicate entry also inflates i_players.length, compounding the wrong-divisor bug in closePot() and locking even more tokens on pot closure

Proof of Concept

The test below registers player1 twice with different reward amounts (300 and 100). The contract is funded for the full 400 total. Only 100 tokens are claimable by player1 because the mapping was overwritten. The remaining 300 tokens are permanently stuck in the contract.

function test_DuplicatePlayerLocksTokens() public mintAndApproveTokens {
// player1 appears twice in the players array
address[] memory dupPlayers = new address[](3);
dupPlayers[0] = player1; // first entry: 300 reward
dupPlayers[1] = player2;
dupPlayers[2] = player1; // duplicate: overwrites to 100 reward
​
uint256[] memory dupRewards = new uint256[](3);
dupRewards[0] = 300;
dupRewards[1] = 50;
dupRewards[2] = 100;
​
uint256 dupTotal = 450; // funded for all 3 entries
​
vm.startPrank(user);
contest = ContestManager(conMan).createContest(
dupPlayers, dupRewards, IERC20(ERC20Mock(weth)), dupTotal
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
// player1 can only claim 100 — first entry (300) was overwritten
uint256 player1Reward = Pot(contest).checkCut(player1);
assertEq(player1Reward, 100);
​
uint256 player1BalanceBefore = ERC20Mock(weth).balanceOf(player1);
vm.prank(player1);
Pot(contest).claimCut();
uint256 player1BalanceAfter = ERC20Mock(weth).balanceOf(player1);
​
// player1 received only 100, not 400 (300 + 100)
assertEq(player1BalanceAfter - player1BalanceBefore, 100);
​
// 300 tokens are permanently stuck in the contract
assertGt(ERC20Mock(weth).balanceOf(contest), 300);
}

Recommended Mitigation

Add a duplicate address check in ContestManager.createContest() before deploying the Pot. Rejecting duplicate entries at the entry point prevents the overwrite from ever reaching the constructor.

// ContestManager.sol
function createContest(
address[] memory players,
uint256[] memory rewards,
IERC20 token,
uint256 totalRewards
) public onlyOwner returns (address) {
+ for (uint256 i = 0; i < players.length; i++) {
+ for (uint256 j = i + 1; j < players.length; j++) {
+ require(players[i] != players[j], "Duplicate player address");
+ }
+ }
Pot pot = new Pot(players, rewards, token, totalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}
Updates

Lead Judging Commences

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