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

`Pot` constructor overwrites rather than accumulates rewards for a duplicated player address, mis-allocating funds and stranding the difference

drak3p14159

Severity: H · Impact: H · Likelihood: M

Description

  • The constructor builds the playersToRewards mapping by assigning each entry directly. It assumes every address in players is unique.

  • If an address appears more than once (a legitimate way to grant someone several reward lines), each assignment overwrites the previous one instead of adding to it. The player can then claim only their last reward line; every earlier allocation is lost and its tokens are stranded in the Pot.

// src/Pot.sol:L32-L34
for (uint256 i = 0; i < i_players.length; i++) {
@> playersToRewards[i_players[i]] = i_rewards[i]; // '=' overwrites on duplicate address; should accumulate
}

Risk

Likelihood

  • Occurs whenever the manager includes the same address twice in players (batching multiple rewards to one recipient — an expected input the code does not forbid or handle).

Impact

  • Mis-allocation and loss of user funds: the duplicated player is credited less than the totalRewards earmarked for them, and the un-credited remainder is locked in the contract (it was funded but is unclaimable).

Proof of Concept

Save as test/PoC_H03.t.sol and run forge test --mt test_duplicatePlayerOverwritesReward -vv. Address A is listed twice with rewards 10 and 20 (30 total), but is credited only 20; 10 is stranded.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import {Test, console} from "lib/forge-std/src/Test.sol";
import {ERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
import {ContestManager} from "../src/ContestManager.sol";
import {Pot} from "../src/Pot.sol";
​
contract MockERC20 is ERC20 {
constructor() ERC20("Mock", "MCK") {}
function mint(address to, uint256 amt) external { _mint(to, amt); }
}
​
contract PoC_H03 is Test {
function test_duplicatePlayerOverwritesReward() public {
ContestManager cm = new ContestManager(); // owner = address(this)
MockERC20 token = new MockERC20();
​
address a = address(0xA11CE);
address[] memory players = new address[](2);
uint256[] memory rewards = new uint256[](2);
players[0] = a; players[1] = a; // same address twice
rewards[0] = 10; rewards[1] = 20; // allocated 10 + 20 = 30
​
token.mint(address(this), 30);
token.approve(address(cm), 30);
address pot = cm.createContest(players, rewards, IERC20(address(token)), 30);
cm.fundContest(0);
​
// src/Pot.sol:L33 overwrote 10 with 20 -> only 20 is credited, not 30
assertEq(Pot(pot).checkCut(a), 20);
​
vm.prank(a); Pot(pot).claimCut();
assertEq(token.balanceOf(a), 20); // A receives 20, not the 30 earmarked
assertEq(token.balanceOf(pot), 10); // 10 funded-but-unclaimable, permanently stuck
console.log("stranded in pot:", token.balanceOf(pot));
}
}

Recommended Mitigation

Accumulate rewards for repeated addresses instead of overwriting.

// src/Pot.sol:L33
- playersToRewards[i_players[i]] = i_rewards[i];
+ playersToRewards[i_players[i]] += i_rewards[i];

Why: += sums every reward line assigned to an address, so a player listed multiple times is credited their full entitlement and no funds are stranded.

Updates

Lead Judging Commences

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