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

Unbound array of players may cause `OutOfGas` error and `DoS` attack

valya

Impact: High
Likelihood: Low

Root + Impact

Description

  • To create a contest, user calls ContestManager::createContest() function, which accepts an array of players of arbitraty length and passes it to a Pot.sol constructor, where for loop iterates over players. If there are more than 14430 members in address[] memory players, the OutOfGas error occurs, contest creation will revert and a lot of gas will be spent.

Here is ContestManager::createContest() function:

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

Here is a Pot.sol 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;
​
// i_token.transfer(address(this), i_totalRewards);
@> for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i];
}
}

Risk

Likelihood:

  • The issue occurs when more than 14430 players are sent to ContestManager::createContest() function.

Impact:

  • If the user does not know about this limitation and attempts to create a contest, the contest will not be created and the gas will be spent.

Proof of Concept

Please add the following fuzz test to the TestMyCut.t.sol. The max amount of totalRewards is type(uint96).max since it is big enough for any balance. The minimum amount of players, when the function ContestManager::createContest() does not revert is 14429, so the test uses 14430 players to show the revert.

function test_unboundPlayersAmount_cause_OutOfGas_and_memoryOOG_fuzz(uint256 totalRewardsFuzz, uint256 playersAmount) public {
uint256 playersAmountWhenOutOfGaz = 14430;
totalRewardsFuzz = bound(totalRewardsFuzz, playersAmountWhenOutOfGaz, type(uint96).max);
playersAmount = bound(playersAmount, 0, playersAmountWhenOutOfGaz); //zero players is tested, 1 wei reward is tested
// arrange owner
uint256 currentUserBalance = ERC20Mock(address(weth)).balanceOf(user);
uint256 wethTotalSupply = weth.totalSupply();
//to prevent overflow on mint consider totalSupply as well
if(currentUserBalance + wethTotalSupply < totalRewardsFuzz){
ERC20Mock(address(weth)).mint(user, totalRewardsFuzz - currentUserBalance - wethTotalSupply);
}
weth.approve(address(conMan), totalRewardsFuzz);
​
uint256 oneReward = playersAmount == 0 ? totalRewardsFuzz : totalRewardsFuzz / playersAmount;
uint256 totalRewardsRounded = playersAmount == 0 ? totalRewardsFuzz : oneReward * playersAmount;
address[] memory playersLocal = new address[](playersAmount);
uint256[] memory rewardsLocal = new uint256[](playersAmount);
​
for(uint256 i = 0; i < playersAmount; i++){
address player = vm.randomAddress();
playersLocal[i] = player;
rewardsLocal[i] = oneReward;
}
vm.prank(user);
ContestManager(conMan).createContest(playersLocal, rewardsLocal, IERC20(ERC20Mock(weth)), totalRewardsRounded);
}

The output of the test above is the following:

├─ [987656084] → new Pot@0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0
│ │ ├─ emit OwnershipTransferred(previousOwner: 0x0000000000000000000000000000000000000000, newOwner: ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353])
@> │ │ └─ ← [OutOfGas] 0x608060405234801561000f575f80fd5b5060043610610086575f3560e01c8063715018a611610059578063715018a6146100ec5780638da5cb5b146100f6578063c946e5dc1461011457

Recommended Mitigation

To avoid this issue it is recommended:

  1. Add the maximum amount of players as a constant.

  2. Add a custom error to notify about the issue.

  3. Check a length of players array when a user attempts to create a new contest.

+ error ContestManager__MaximumAmountOfPlayersExceeded();
+ uint256 public constant MAXIMUM_AMOUNT_OF_PLAYERS = 1429;
​
function createContest(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards)
public
onlyOwner
returns (address)
{
+ require(players.length < MAXIMUM_AMOUNT_OF_PLAYERS, ContestManager__MaximumAmountOfPlayersExceeded());
// Create a new Pot contract
Pot pot = new Pot(players, rewards, token, totalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}

Also consider that it may be too gas intensive to create a contes and expenses on gas will not be covered by manager's cut, which may be even zero.

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!