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

No mechanism to prevent repeated calls of `ContestManager::closeContest()` or to destroy the `Pot`

cryptolappi

Summary

The ContestManager contract allows the owner to call the closeContest function multiple times for the same contest, as there is no mechanism to prevent repeated calls or to destroy the Pot contract after it has been closed.

Vulnerability Details

In the ContestManager contract, the closeContest function can be called by the owner for any contest:

function closeContest(address contest) public onlyOwner {
_closeContest(contest);
}
​
function _closeContest(address contest) internal {
Pot pot = Pot(contest);
pot.closePot();
}

The issue arises because:

  1. There is no check to see if the contest has already been closed.

  2. The Pot contract is not destroyed after closing.

  3. There is no state variable in either ContestManager or Pot to track whether a contest has been closed.

In the Pot contract, the closePot function distributes remaining rewards but does not implement any mechanism to prevent multiple calls:

function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
​
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

Impact

The ability to call closeContest multiple times can lead to several issues:

  1. Double spending: If there are any remaining rewards, they could be distributed multiple times, potentially draining the contract of more funds than intended.

  2. Incorrect accounting: The remainingRewards in the Pot contract might become inconsistent with the actual token balance.

  3. Confusion for users: Users might see multiple closing events for the same contest, leading to uncertainty about the contest's status.

  4. Gas waste: Unnecessary gas costs for repeated closing operations.

The severity of this issue is high, as it can lead to financial losses and compromise the integrity of the contest system.

Tools Used

Manual code review.

AI for report text and formatting.

Recommendations

To address this vulnerability, consider implementing the following changes:

  1. Add a state variable in the Pot contract to track whether it has been closed:

bool private isClosed;
​
function closePot() external onlyOwner {
require(!isClosed, "Pot has already been closed");
// ... existing close logic ...
isClosed = true;
}

  1. Implement a self-destruct mechanism in the Pot contract to prevent any further interactions after closing:

function closePot() external onlyOwner {
require(!isClosed, "Pot has already been closed");
// ... existing close logic ...
isClosed = true;
selfdestruct(payable(owner()));
}

  1. Update the ContestManager contract to remove the closed contest from its list and mapping:

function closeContest(address contest) public onlyOwner {
_closeContest(contest);
// Remove the contest from the list and mapping
for (uint i = 0; i < contests.length; i++) {
if (contests[i] == contest) {
contests[i] = contests[contests.length - 1];
contests.pop();
break;
}
}
delete contestToTotalRewards[contest];
}

  1. Consider adding an event to log when a contest is closed, providing transparency to users:

event ContestClosed(address indexed contestAddress);
​
function closeContest(address contest) public onlyOwner {
_closeContest(contest);
// ... removal logic ...
emit ContestClosed(contest);
}

By implementing these recommendations, you can prevent multiple closings of the same contest and ensure the integrity of the contest management system.

Updates

Lead Judging Commences

equious Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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