MyCut

AI First Flight #8
Beginner FriendlyFoundry
EXP
View results
Submission Details
Severity: low
Valid

No validation that players.length == rewards.length or that sum(rewards) == totalRewards, allowing an insolvent Pot where some players cannot claim

Root + Impact

Description

ContestManager.createContest() and the Pot constructor accept players, rewards, and totalRewards with no consistency checks:

// Pot.sol constructor
for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i];
}

There is no check that:

  1. players.length == rewards.length - if rewards is shorter, the loop reverts with an out-of-bounds access (Pot cannot be created); if longer, extra rewards are silently ignored.

  2. sum(rewards) == totalRewards - remainingRewards is initialized to totalRewards and fundContest transfers exactly totalRewards, but per-player payouts come from rewards. If sum(rewards) > totalRewards, the Pot is insolvent: early claimers drain it and later players' claimCut() reverts (token transfer fails / remainingRewards -= reward underflows).

Risk

Likelihood: Medium - any time the admin sets rewards/totalRewards inconsistently (no on-chain guard prevents it).

Impact: Low - deposited funds are safe, but affected players cannot claim their promised rewards (DoS of the claim path); requires admin misconfiguration so it is not directly attacker-driven.

Proof of Concept

rewards sum to 6 but the pot is only funded with 4, so the second claimer cannot be paid. Runnable Foundry test (drop into test/TestMyCut.t.sol):

function test_PoC_noRewardSumValidation() public mintAndApproveTokens {
// rewards sum to 6, but totalRewards/funding is only 4 -> insolvent pot
vm.startPrank(user);
rewards = [3, 3];
totalRewards = 4;
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
// player1 claims 3 (ok); the pot now holds only 1
vm.prank(player1);
Pot(contest).claimCut();
// player2 is promised 3 but only 1 remains -> claim reverts, reward inaccessible
vm.prank(player2);
vm.expectRevert();
Pot(contest).claimCut();
}

Run forge test --mt test_PoC_noRewardSumValidation -vv; it passes, showing player2 cannot claim.

Recommended Mitigation

Validate the inputs before deploying the Pot so an inconsistent contest can never be created. Require that the arrays line up and that the promised rewards exactly equal the funded total. Apply this in ContestManager.createContest() (and/or the Pot constructor):

function createContest(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards)
public
onlyOwner
returns (address)
{
require(players.length == rewards.length, "players/rewards length mismatch");
uint256 sum;
for (uint256 i = 0; i < rewards.length; i++) {
sum += rewards[i];
}
require(sum == totalRewards, "rewards must sum to totalRewards");
Pot pot = new Pot(players, rewards, token, totalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}

This guarantees every player has a funded reward and the pot is always solvent, eliminating both the out-of-bounds revert and the insolvency/DoS condition.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[L-01] The logic for ContestManager::createContest is NOT efficient

## Description there are two major problems that comes with the way contests are created using the `ContestManager::createContest`. - using dynamic arrays for `players` and `rewards` leads to potential DoS for the `Pot::constructor`, this is possible if the arrays are too large therefore requiring too much gas - it is not safe to trust that `totalRewards` value supplied by the `manager` is accurate and that could lead to some players not being able to `claimCut` ## Vulnerability Details - If the array of `players` is very large, the `Pot::constructor` will revert because of too much `gas` required to run the for loop in the constructor. ```Solidity 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; // i_token.transfer(address(this), i_totalRewards); @> for (uint256 i = 0; i < i_players.length; i++) { @> playersToRewards[i_players[i]] = i_rewards[i]; @> } } ``` - Another issue is that, if a `Pot` is created with a wrong `totalRewards` that for instance is less than the sum of the reward in the `rewards` array, then some players may never get to `claim` their rewards because the `Pot` will be underfunded by the `ContestManager::fundContest` function. ## PoC Here is a test for wrong `totalRewards` ```solidity function testSomePlayersCannotClaimCut() public mintAndApproveTokens { vm.startPrank(user); // manager creates pot with a wrong(smaller) totalRewards value- contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), 6); ContestManager(conMan).fundContest(0); vm.stopPrank(); vm.startPrank(player1); Pot(contest).claimCut(); vm.stopPrank(); vm.startPrank(player2); // player 2 cannot claim cut because the pot is underfunded due to the wrong totalScore vm.expectRevert(); Pot(contest).claimCut(); vm.stopPrank(); } ``` ## Impact - Pot not created if large dynamic array of players and rewards is used - wrong totlRewards value leads to players inability to claim their cut ## Recommendations review the pot-creation design by, either using merkle tree to store the players and their rewards OR another solution is to use mapping to clearly map players to their reward and a special function to calculate the `totalRewards` each time a player is mapped to her reward. this `totalRewards` will be used later when claiming of rewards starts.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!