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

Pot.sol::closePot() Dangerously Assumes msg.sender is the Contest Manager

nadir_khan_sec

Summary

Pot.sol::closePot() assumes the caller of the function is the contest manager. If the contract is ever hacked in some way granting owner access to the attacker, the msg.sender will be able to obtain the manager cut by calling Pot.sol::closePot()

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); //@audit - should transfer to MANAGER not msg.sender. Despite modifier..if ever it's broken somehow to be safer explicitly ID the manager address
​
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

Impact

If the contract is ever hacked in some way granting owner access to the attacker, the msg.sender will be able to obtain the manager cut by calling Pot.sol::closePot()

Tools Used

Manual Review

Recommendations

Consider creating a distinct role for the manager using OwnableRoles (who may not always be the owner). Create a mechanism to assign or change managers (onlyManagerRole) and change Pot.sol::Line 7 to safeTransfer (instead of transfer) to the manager address rather than msg.sender. This required changing ContestManager.sol::createContest to assign the manager's role to an address. This ensures that if the owner is hacked in some way, the manager still retains the ability to call the closePot and receive the manager's cut.

Updates

Lead Judging Commences

equious Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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