First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

withdrawFees should only be able to be called by the owner of the contract (or a trusted third party they have delegated to)

happyformerlawyer

Summary

withdrawFees can be called by anyone. There is no onlyOwner modifier. What if, eg, the contract owner lost the private keys to the current feeAddress and wanted to update it but a third party called withdrawFees before the owner had a chance to do so? There is no good reason that anyone other than the contract owner (or someone they have delegated to) should be able to call withdrawFees.

Vulnerability Details

function withdrawFees() external {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

Impact

The fees could end up locked in the contract if the keys for the fee address have been lost and a third party calls withdrawFees before feeAddress can be updated. Also, it should be the owner's control when the fees are withdrawn because it is his contract.

Tools Used

Manual review

Recommendations

Add an onlyOwner modifier. You have already imported from Ownable.sol from OpenZeppelin so you don't even need to write your own modifier:

function withdrawFees() external onlyOwner {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

If you want a third party to also be able to withdraw, you could either create a modifier onlyOwnerOrDelegate with a check that the calling address is in an array of permitted addresses. If you do this, you could also create the ability to add or remove addresses if you want.

Updates

Lead Judging Commences

Hamiltonite Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: User experience and design improvement

Support

FAQs

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