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

Pot is created without checking that the rewards add up to `totalRewards`, so some players cannot claim their reward

theo_mc

Root + Impact

ContestManager::createContest and the Pot constructor never check that sum(rewards) == totalRewards. fundContest sends only totalRewards tokens to the Pot. If the rewards add up to more than totalRewards, the Pot does not hold enough tokens for all promised rewards. The player who claims last cannot claim: claimCut reverts with an arithmetic underflow, and the player's reward can never be paid.

Description

The constructor stores the rewards and the total separately and never compares them:

constructor(
address[] memory players,
uint256[] memory rewards,
IERC20 token,
uint256 totalRewards
) {
...
remainingRewards = totalRewards;
...
​
// no check that sum(rewards) == totalRewards
for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i];
}
}

Pot::claimCut subtracts each reward from Pot::remainingRewards, so when the promised rewards are more than the total, the last claim goes below zero and reverts:

remainingRewards -= reward; // underflow when the rewards add up to more than totalRewards

Example: rewards 10 / 20 / 40 (70 promised), totalRewards = 60. After player1 and player2 claim, 30 tokens remain, but player3 is owed 40. player3's claimCut reverts and player3 gets nothing.

Risk

Likelihood: Low

The owner must pass numbers that do not add up (for example, by a typing mistake). No attacker is involved.
Nothing in the code or in fundContest warns about it, so the mistake is not noticed before players try to claim.

Impact: Medium

One or more players cannot claim the reward they were promised. In the PoC, player3 is owed 40 and gets 0.
The protocol's own documentation says players are "authorized" to claim these amounts, so this breaks the promise.
Only the money of this one contest is affected.

Proof of Concept

Add the following functions inside the existing TestMyCut contract in test/TestMyCut.t.sol.

address player3 = makeAddr("player3");
​
function _createFundedPot(
uint256 r1,
uint256 r2,
uint256 r3,
uint256 total
) internal returns (Pot) {
address[] memory p = new address[](3);
p[0] = player1;
p[1] = player2;
p[2] = player3;
​
uint256[] memory r = new uint256[](3);
r[0] = r1;
r[1] = r2;
r[2] = r3;
​
vm.startPrank(user);
contest = ContestManager(conMan).createContest(
p,
r,
IERC20(ERC20Mock(weth)),
total
);
​
ContestManager(conMan).fundContest(0);
​
vm.stopPrank();
​
return Pot(contest);
}
​
function test_PoC_rewardsSumExceedsTotal() public mintAndApproveTokens {
Pot pot = _createFundedPot(10, 20, 40, 60); // 70 promised, 60 funded
​
vm.prank(player1);
pot.claimCut(); // remainingRewards = 50
​
vm.prank(player2);
pot.claimCut(); // remainingRewards = 30
​
vm.prank(player3);
vm.expectRevert(
abi.encodeWithSignature("Panic(uint256)", uint256(0x11))
); // remainingRewards (30) -= 40 underflows
pot.claimCut();
​
assertEq(pot.checkCut(player3), 40);
assertEq(weth.balanceOf(player3), 0);
assertEq(weth.balanceOf(address(pot)), 30);
​
uint256 remainingRewards = ContestManager(conMan)
.getContestRemainingRewards(address(contest));
​
console.log("player3 is owed:", pot.checkCut(player3));
console.log("player3 received:", weth.balanceOf(player3));
console.log("tokens left in the Pot:", weth.balanceOf(address(pot)));
console.log("Remaining rewards", remainingRewards);
​
assertEq(remainingRewards, 30);
}

Run from the repository root:

forge test --mt test_PoC_rewardsSumExceedsTotal -vv

Output:

[PASS] test_PoC_rewardsSumExceedsTotal() (gas: 1184922)
Logs:
...
player3 is owed: 40
player3 received: 0
tokens left in the Pot: 30
Remaining rewards 30

Recommended Mitigation

The Pot must never be created with rewards that do not add up to totalRewards. If this is guaranteed at creation, the Pot always holds exactly the amount that is promised (because fundContest sends totalRewards), and every claimCut call can be paid: remainingRewards and the sum of the unclaimed rewards decrease by the same amount, so the subtraction can never underflow.

The simplest place for this check is ContestManager::createContest, before the Pot is deployed. A wrong input then fails early, before any Pot is created:

+ error ContestManager__RewardsMismatch();
​
function createContest(
address[] memory players,
uint256[] memory rewards,
IERC20 token,
uint256 totalRewards
) public onlyOwner returns (address) {
+ uint256 sum;
+ for (uint256 i = 0; i < rewards.length; i++) {
+ sum += rewards[i];
+ }
+ if (sum != totalRewards) revert ContestManager__RewardsMismatch();
// Create a new Pot contract
...
}

The check uses != and not >, so it also covers the opposite case, sum(rewards) < totalRewards. There, the extra tokens would stay in the Pot and belong to nobody.

The same check can also be added in the Pot constructor, so the invariant holds for every way a Pot can be created.

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!