MyCut

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

[M-1] the miss connecting between `totalrewards` and `rewards` can lead to arthematic overflow or underflow

[M-1] the miss connecting between totalrewards and rewards can lead to arthematic overflow or underflow

Description

  • the totalReward has no check with rewards array that can lead to underflow if the total rewards less than the sum of the rewards , that will make this pot unusable again and need to add new one , that will make it loss funds too


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; // question : what if totalRewards != sum(rewards) ?
i_deployedAt = block.timestamp;
// note :transfer the total rewards from the contest manager to this pot
// i_token.transfer(address(this), i_totalRewards); // commented
for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i]; // map each player to their reward
}
}

Risk

Likelihood:

  • occures if the owner not set the same sum of rewards with the totalRewards , with no checkers


Impact:

  • can lead to underflow what makes the pot unusable and loss rest of funds , or cannot close it

Proof of Concept

....
uint256[] rewards = [3, 2];
address user = makeAddr("user");
uint256 totalRewards = 4;
...
function testCanClaimCut() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), 4);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
// player balance before
uint256 balanceBefore = ERC20Mock(weth).balanceOf(player1);
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
// player balance after
uint256 balanceAfter = ERC20Mock(weth).balanceOf(player1);
assert(balanceAfter > balanceBefore);
vm.prank(player2);
Pot(contest).claimCut();
uint256 balanceAfter2 = ERC20Mock(weth).balanceOf(player2);
console.log("player2 balance after claim:", balanceAfter2);
assert(balanceAfter2 == 2);
}
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [938] ERC20Mock::balanceOf(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84]) [staticcall]
│ ├─ [223] ERC20::balanceOf(0x7026B763CBE7d4E72049EA67E89326432a50ef84)
│ │ └─ ← 3
│ └─ ← [Return] 3
├─ [0] VM::prank(player2: [0xEb0A3b7B96C1883858292F0039161abD287E3324])
│ └─ ← [Return]
├─ [944] Pot::claimCut()
│ └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
└─ ← [Revert] panic: arithmetic underflow or overflow (0x11)

Recommended Mitigation

constructor(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards) {
i_players = players;
i_rewards = rewards;
i_token = token;
i_totalRewards = totalRewards
+ require(totalRewards == sum(rewards),"lack of total rewards re check")
remainingRewards = totalRewards; // question : what if totalRewards != sum(rewards) ?
i_deployedAt = block.timestamp;
// note :transfer the total rewards from the contest manager to this pot
// i_token.transfer(address(this), i_totalRewards); // commented
for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i]; // map each player to their reward
}
}
Updates

Lead Judging Commences

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