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

Pot Constructor Overwrites Rewards for Duplicate Players — Leads to Incorrect Distribution

bamgboseoluwole83

Root + Impact

Description

  • Describe the normal behavior in one or more sentences

  • Explain the specific issue or problem in one or more sentences

// R# [M-#] Pot Constructor Overwrites Rewards for Duplicate Players — Leads to Incorrect Distribution
​
## Summary
The `Pot` constructor maps input user records into the `playersToRewards` storage ledger utilizing a direct replacement setup. If an identical user account entry appears multiple times within the input collection array, each iteration replaces previous data values rather than summing them together. This causes affected accounts to receive fewer prize distributions than intended, while leaving the leftover token supply unmapped and permanently trapped inside the contract.
​
## Vulnerability Details
Inside `Pot.sol`, the initialization logic routes over raw input arrays inside a loop structure:
```solidity
for (uint256 i = 0; i < _players.length; i++) {
playersToRewards[_players[i]] = _rewards[i];
}
```
​
If an administrative compilation routine error or manual input issue lists a target account multiple times inside the `_players` collection, the mapping index continuously overwrites previous balance assignments.
​
Because the initial values are overwritten, only the token allocation from the final array index becomes claimable. However, the contract's expected total funding requirement calculations integrate the aggregate sum of all array items. As a result, the unassigned token value differences remain entirely out of reach of the user mapping registry, trapping the excess funds inside the contract balance with no recovery mechanism.
​
## Impact
**Medium.** Legitimate contest participants lose a direct portion of their prize pool allocation when accidental duplicate address arrays are provided during initialization. The residual undistributed capital stays locked within the contract balance indefinitely.
​
## Proof of Concept
Add the following test case to your test script to verify the assignment override:
​
```solidity
function test_DuplicatePlayerOverwritesReward() public {
address[] memory players = new address[](3);
players[0] = alice;
players[1] = bob;
players[2] = alice; // @audit Alice is added twice
​
uint256[] memory rewards = new uint256[](3);
rewards[0] = 100e18;
rewards[1] = 50e18;
rewards[2] = 200e18;
​
// Initialize Pot contract requiring 350e18 total fund backing
Pot pot = new Pot(players, rewards, IERC20(token), 350e18);
// Alice's final claimable balance reflects only the last index value
uint256 aliceReward = pot.checkCut(alice);
assertEq(aliceReward, 200e18); // Lost the original 100e18 allocation
// 100e18 tokens remain permanently frozen in the contract balance
}
```
​
## Tools Used
* Manual Code Review
​
## Recommendations
Refactor the looping configuration inside the constructor to accumulate incoming reward allocations for existing accounts instead of performing direct value replacements:
​
```solidity
for (uint256 i = 0; i < _players.length; i++) {
// @audit-issue Accumulate allocations instead of overwriting to handle duplicates safely
playersToRewards[_players[i]] += _rewards[i];
}
```
Alternatively, if duplicate player entries are strictly forbidden by protocol specifications, insert an early validation check to revert the transaction:
```solidity
for (uint256 i = 0; i < _players.length; i++) {
if (playersToRewards[_players[i]] > 0) {
revert Pot__DuplicatePlayerNotAllowed();
}
playersToRewards[_players[i]] = _rewards[i];
}
```
oot cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • Reason 1 // Describe WHEN this will occur (avoid using "if" statements)

  • Reason 2

Impact:

  • Impact 1

  • Impact 2

Proof of Concept

​

Recommended Mitigation

- remove this code
+ add this code
Updates

Lead Judging Commences

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