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

Pot::Constructor is using an unbounded for loop to map the players to rewards, which may lead to high gas costs and eventually DoS

dharmin

Description

In ContestManager::createContest when initializing a new Pot, the constructor receives an array of players. This approach is efficient for small inputs, but as the number of players grows, the gas cost increases drastically.

ContestManager::createContest

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

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

Impact

An attacker could exploit this to initiate a Denial of Service (DoS) attack, causing gas costs to skyrocket and making it prohibitively expensive to create a contest.

Proof of Concepts

If we host the contest for 100 players, the gas costs will be as such: ~7260133 gas

If we host the contest for 1000 players, the gas costs will be as such: ~68585788 gas

This is more than 9x more expensive for hosting the contest for 1000 players and this will keep on increasing

Proof of Code:

function testDoSInConstructor() public {
address[] memory playersPot = new address[](100);
uint256[] memory rewardsPot = new uint256[](100);
uint256 totalRewardsPot = 100;
uint256 gasStart = gasleft();
for (uint256 i = 0; i < 100; i++) {
playersPot[i] = address(uint160(i));
rewardsPot[i] = 1;
}
Pot pot = new Pot(playersPot, rewardsPot, IERC20(ERC20Mock(weth)), totalRewardsPot);
uint256 gasEnd = gasleft();
uint256 gasUsed = gasStart - gasEnd;
console.log("Gas used: %d", gasStart - gasEnd);
​
address[] memory players2 = new address[](1000);
uint256[] memory rewards2 = new uint256[](1000);
uint256 totalRewards2 = 1000;
gasStart = gasleft();
for (uint256 i = 0; i < 1000; i++) {
players2[i] = address(uint160(i));
rewards2[i] = 1;
}
Pot pot2 = new Pot(players2, rewards2, IERC20(ERC20Mock(weth)), totalRewards2);
gasEnd = gasleft();
uint256 gasUsed2 = gasStart - gasEnd;
console.log("Gas used: %d", gasStart - gasEnd);
​
assert(gasUsed2 > gasUsed);
}

Recommended mitigation

Instead of receiving a whole players array at once, let each player have option to register themselves so that the gas cost to map the players is distributed across multiple transactions

Updates

Lead Judging Commences

equious Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Unbound for loop in Contest Creation

Support

FAQs

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