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

Possible discrepancy between sum of all rewards and `totalRewards` causes incorrect accounting and may lead to underflow error

valya

Impact: High
Likelihood: High

Root + Impact

Description

  • To create a contest, user should pass an array of rewards for every player uint256[] memory rewards and the total rewards amount uint256 totalRewards. The total rewards amount is assigned to remainingRewards in the constructor of Pot.sol and this amount is decremented on every player's claim. If the sum of rewards from an array is not the same as totalRewards amount, the incorrect accounting and possible underflow will be when Pot::claimCut() and Pot::closePot() are called.

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];
}
}
​
function claimCut() public {
address player = msg.sender;
uint256 reward = playersToRewards[player];
if (reward <= 0) {
revert Pot__RewardNotFound();
}
playersToRewards[player] = 0;
@> remainingRewards -= reward;
claimants.push(player);
_transferReward(player, reward);
}

Risk

Likelihood:

  • The issue occurs when the sum of rewards is not equal to totalRewards parameter in ContestManager::createContest() function.

Impact:

  • Depending of whether totalRewards is less or more than the sum of rewards in the array uint256[] memory rewards, the following scenarios may develop (remember that totalRewards value is assigned to remainingRewards in a Pot):


    • if (totalRewards < sum of rewards) - possible underflow in Pot::claimCut() when remainingRewards -= reward;;


    • if (totalRewards > sum of rewards) - the remaining amount will be more than it should be, then wrong 10% of manager's cut and wrong claimant cut will be sent to participants.

Proof of Concept

Please, add the following test to TestMyCut.t.sol to see that the issue causes:

  1. panic: arithmetic underflow or overflow when totalRewards < sum of rewards

  2. wrong calculating of remainingRewards and managersCut when totalRewards > sum of rewards

function test_totalRewardAndRewardsSumDiscrepancy() public mintAndApproveTokens {
// contest with totalRewards < sum of rewards
vm.startPrank(user);
rewards = [500, 500];
totalRewards = 950;
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
​
vm.startPrank(player2);
vm.expectRevert(stdError.arithmeticError); // reverts with 'panic: arithmetic underflow or overflow'
Pot(contest).claimCut();
vm.stopPrank();
​
// contest2 with totalRewards > sum of rewards
vm.startPrank(user);
rewards = [500, 500];
totalRewards = 1950;
address contest2 = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(1);
vm.stopPrank();
​
vm.startPrank(player1);
Pot(contest2).claimCut();
vm.stopPrank();
​
uint256 expectedRemainingRewards = 500;
uint256 actualRemainingRewards = Pot(contest2).getRemainingRewards();
assertGt(actualRemainingRewards, expectedRemainingRewards);
​
vm.warp(91 days);
​
// contest manager balance before close contest
uint256 conManBalanceBeforeCloseContest = ERC20Mock(weth).balanceOf(conMan);
​
vm.prank(user);
ContestManager(conMan).closeContest(contest2);
​
// contest manager balance after close contest
uint256 conManBalanceAfterCloseContest = ERC20Mock(weth).balanceOf(conMan);
​
uint256 managerCutPercent = 10;
uint256 expectedManagerCut = expectedRemainingRewards / managerCutPercent;
uint256 actualManagerCut = conManBalanceAfterCloseContest - conManBalanceBeforeCloseContest;
assertGt(actualManagerCut, expectedManagerCut);
}

Recommended Mitigation

It will be more reliable to calculate remainingRewards in Pot.sol constructor based on values of rewards in rewards array rather than pass totalRewards to ContestManager::createContest() function and Pot.sol constructor:

Consider these changes in Pot.sol:

- uint256 private immutable i_totalRewards;
​
- constructor(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards) {
+ constructor(address[] memory players, uint256[] memory rewards, IERC20 token) {
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];
+ remainingRewards += i_rewards[i];
}
}

Consider these changes in ContestManager::createContest():

-function createContest(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards)
+function createContest(address[] memory players, uint256[] memory rewards, IERC20 token)
public
onlyOwner
returns (address)
{
- Pot pot = new Pot(players, rewards, token, totalRewards);
+ Pot pot = new Pot(players, rewards, token);
​
contests.push(address(pot));
- contestToTotalRewards[address(pot)] = totalRewards;
+ contestToTotalRewards[address(pot)] = pot.getRemainingRewards()
return address(pot);
}
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!