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

It is possible to fund the contest multiple times or not at all.

ollam

Summary

There are no checks to ensure that a contest only been funded once or if it has been funded at all.

Vulnerability Details

fundContest() only reverts if the sender does not have funds and has no checks for anything else.

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); // pot created with no funds - users may try to interact and have their transactions revert
}
​
function fundContest(uint256 index) public onlyOwner {
Pot pot = Pot(contests[index]);
IERC20 token = pot.getToken();
uint256 totalRewards = contestToTotalRewards[address(pot)];
​
if (token.balanceOf(msg.sender) < totalRewards) {
revert ContestManager__InsufficientFunds();
}
// possible to transfer funds over and over again
@> token.transferFrom(msg.sender, address(pot), totalRewards);
}

Impact

There are two potential issues that arise because this.

  1. It is possible to create a contest and never fund it. Users would be able to interact with the contest, likely under the assumption that they would be able to claim the rewards that are due to them as viewable in Pot::playersToRewards. This would cause reverting transactions and wasted gas on the users' end.

  2. In the event that the manager of the protocol calls this function multiple times, the excess funds would be lost. In the Pot contract, only totalRewards is used to distribute the remainder of rewards when the pot is closed. Sending additional funds does not change totalRewards and there is no rescue function, ensuring loss to the manager should this be called more than once.

PoC - copy this into TestMyCut.t.sol:

function testMultipleFunds() public {
vm.startPrank(user);
weth = new ERC20Mock("WETH", "WETH", user, 1000e8);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), 4);
weth.approve(conMan, 1000e8);
ContestManager(conMan).fundContest(0);
ContestManager(conMan).fundContest(0);
ContestManager(conMan).fundContest(0);
ContestManager(conMan).fundContest(0);
ContestManager(conMan).fundContest(0);
ContestManager(conMan).fundContest(0);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
assertEq(ERC20Mock(weth).balanceOf(contest), 28);
​
vm.startPrank(player1);
Pot(contest).claimCut();
vm.startPrank(player2);
Pot(contest).claimCut();
vm.warp(91 days);
​
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
assertEq(ERC20Mock(weth).balanceOf(contest), 24);
}

Tools Used

Manual review

Recommendations

Consider making fundContest() a private function that is called at the end of createContest(). This would ensure that all pots created are sufficiently funded and removes the risk of overfunding a pot.

Updates

Lead Judging Commences

equious Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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