Submission Details
Severity:
**[H-3] No validation on `players` and `rewards` within `ContestManager:createContest` or `Pot:constructor`**
**Description:** By not validating the `players` and `rewards` array to contain appropriate values, i.e. forbidding the zero-address, ensuring non-zero values for the reward in addition to ensuring the lengths of the arrays are the same.
The `Pot:constructor` below, specifically the for-loop will reveral this bug:
```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;
// This line is commented, is this supposed to be uncommented?
// i_token.transfer(address(this), i_totalRewards);
for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i];
}
}
```
**Impact:** In the event that `players.length > rewards.length` we will get a reversion in the `Pot:constructor` as the rewards index will become out of bounds. The pot will not create which is a reasonable reversion but it should be more explicit. In the event that `players.length < rewards.length` , unclaimable funds the sum of funds at `index > players.length` within `rewards` will not be claimable by any user but will have been sent to the contract, most likely these will be shared amongst claimants when the pot is closed.
**Proof of Concept:**
Add the following functions to the `TestMyCut.t.sol:TestMyCut` test contract suite.
```solidity
/**
* If the number of users exceeds the length of the rewards array we expect a reversion.
*/
function testRevertWhenLengthUsersGreaterThanRewards() public {
// Arrange - Length of (Players: 2, Rewards: 1)
rewards = [50];
totalRewards = 50;
vm.startPrank(user);
ContestManager contestManager = ContestManager(conMan);
// Act / Assert
vm.expectRevert();
contestManager.createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
}
/**
* If the rewards array has a greater length than the players array, the rewards at index >= players.length
* will be unclaimable by any user until the pot is closed, it will be shared amongst claimants and the conMan.
*/
function testFundsAreAllocatedToUnclaimableUser() public mintAndApproveTokens {
// Arrange
address[] memory contestants = new address[](2); // Length 2
contestants[0] = makeAddr("PlayerOne");
contestants[1] = makeAddr("PlayerTwo");
rewards = [5, 5, 10]; // Length 3 (The 10 WETH is not allocated to a specific user)
totalRewards = 20;
vm.startPrank(user);
ContestManager contestManager = ContestManager(conMan);
contest = contestManager.createContest(contestants, rewards, IERC20(ERC20Mock(weth)), totalRewards);
contestManager.fundContest(0);
Pot contestPot = Pot(contest);
vm.stopPrank();
// Act - All contestants a part of the contest have claimed, yet 10 ETH is still remaining.
for (uint256 i = 0; i < contestants.length; i++) {
vm.prank(contestants[i]);
contestPot.claimCut();
}
// Assert
uint256 remainingRewards = contestPot.getRemainingRewards();
assert(remainingRewards == rewards[rewards.length - 1]); // Unclaimable by any user until contest is closed.
}
```
**Recommended Mitigation:**
Ensure the lengths of both arrays are equal.
```diff
function createContest(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards)
public
onlyOwner
returns (address)
{
// Create a new Pot contract
+ require(players.length == rewards.length, "Each player should have an associated reward.")
Pot pot = new Pot(players, rewards, token, totalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}
```
Rewards breakdown
nSLOC
106This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.