MyCut
First Flight #23
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Player unable to claim cut if `pot.totalRewards` < total amounts in `rewards[]`, due to underflow revert

MrCrowNFT

Summary

When a pot is created, the totalRewards is set, this parameter doesn't have any requirement, therefor it can be less than the actual total rewards amount in rewards[] or even 0, and in order to claim their prizes, the players use Pot::claimCut which substract the amount claimed to remainingRewardsthat in on itself is set as equal to totalRewards when the pot is created, If the said variable is less than the amount claimed, the function will revert beacuse of underflow, and thus the player will not be able to claim his cut.

Vulnerability Details

First in ContestManager::createContest the total rewards is set with no check for zero amount in place, nor check if the said value is equivalent to the actual total rewards (as shown in th code snipet below), as such, it is possible to make a mistake by accident or on purpuse by malicious contest manager.

function createContest(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards)
public
onlyOwner
returns (address)
{
// Create a new Pot contract
Pot pot = new Pot(players, rewards, token, totalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}

When the pot is created, remainingRewards is set to total rewards in Pot::constructor.

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;
​
for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i];
}
}

The issue becomes evident when a player tries to claim his cut, as shown below, the amount the player is claimming, is subtracted from the remainingRewards variable, as susch, if reward > remainingRewards the function will revert due to arithmetic underflow, making it imposible for a player to get his reward.

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);
}

You may corroborate the problem by adding the following codes in TestMyCut.sol, in this codes, we test two posible sitations, the first one where totalRewards = 0 making every claim imposible; an a second one, where totalRewards != 0 but is still less than the actual total amount to be claimed, meaning the first claim will succeed but the following ones will not.

function testCreatePotTotalRewardZero() public mintAndApproveTokens {
//First we create a pot with totalRewards == 0
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), 0);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
//Secondly, player1 tries to claim his cut without success
vm.startPrank(player1);
vm.expectRevert();
Pot(contest).claimCut();
vm.stopPrank();
}
function testCreatePotTotalRewardLessThanRealAmount() public mintAndApproveTokens {
//First we create a pot with totalRewards < Real total rewards (4)
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), 3);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
//Secondly, player1 claims his cut successfully
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
//Finally, player2 tries to claim his cut without success
vm.startPrank(player2);
vm.expectRevert();
Pot(contest).claimCut();
vm.stopPrank();
}

Impact

Player would be unable to claim their prizes if it is greater than the totalRewards if t was set incorrectly.

Tools Used

Foundry and Manual review

Recommendations

The simplest solution would be that the totalRewards variable is set as the addition of all values within the rewards array using a for loop as follows:

- 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)
{
+ uint256 totalRewards = 0
+ for (uint256 i = 0; i < claimants.length; i++){
+ totalRewards = totalRewards + rewards[i];
+ }
+ if (totalRewards == 0){
+ revert;
+ }
Pot pot = new Pot(players, rewards, token, totalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}
Updates

Lead Judging Commences

equious Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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