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

Possible reentrancy attack in Pot::closePot() through the owner

christian14201522

Summary

The Pot::closePot has a possible reentrancy path for the trusted owner, which makes it less critical. However, a trusted owner doesn't guarantee safety. There is still a potential for accidental mistakes or vulnerabilities. An external or third-party contract could interact with the owner's contract, potentially exploiting the vulnerability through indirect means. Additionally, contracts can change ownership or be transferred, so a trusted owner today might not be a trusted owner in the future. Also if someone gets the control over the owners account, the funds are at direct risk.

Vulnerability Details

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

The reentrancy attack could follow this pattern:

  1. The owner is not trusted anymore

  2. calling closePot()

  3. the function transfers the managerCut to the owners fallback function

  4. the fallback-function is malicious, calling closePot() again

  5. whenremainingRewards > 0 the same amount of tokens are transferred again

  6. this is repeated until remainingRewards is empty or the contest is actual empty

Impact

Loss of the contests funds

Tools Used

Manual review

Recommendations

Security best practices, such as the Checks-Effects-Interactions (CEI) pattern and ReentrancyGuard, are implemented to protect against reentrancy vulnerabilities even if the owner is trusted. Using OpenZeppelin's ReentrancyGuard and the CEI pattern protects the contract from unintended behaviors or future vulnerabilities. Even if the current state seems safe, following these patterns ensures long-term security and reduces the potential attack surface.

In this case, claimantCut (majority of tokens) should be transferred first, and then the manager receives their cut, including any remaining tokens.

Updates

Lead Judging Commences

equious Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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