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

Absence of onlyOwner Access Control in withdrawFees Function Leads to Unauthorized Fund Withdrawals

Osora9

Summary

The PuppyRaffle contract has a function withdrawFees which lacks necessary access restrictions, allowing any external actor to potentially withdraw the accumulated fees. This poses a severe financial risk as malicious actors can drain funds meant for the owner.

Vulnerability Details

In the PuppyRaffle contract, the withdrawFees function allows for the withdrawal of all accumulated fees. However, the function doesn't have an onlyOwner modifier or equivalent access control mechanism, permitting anyone to trigger the function and redirect the fees to an address of their choosing.

Impact

This vulnerability can result in financial loss for the contract owner as accumulated fees can be withdrawn by any unauthorized actor. Depending on the popularity and usage of the PuppyRaffle contract, this could represent significant amounts of Ether.

POC

  1. Deploy the PuppyRaffle and Attack contracts.

  2. Interact with the PuppyRaffle contract to accumulate some fees.

  3. Call the changeFeeAddress function of the PuppyRaffle contract to set the fee address to the address of the Attack contract (or any malicious address).

  4. Trigger the attack to withdraw the accumulated fees.

Attack

contract Attack {
PuppyRaffle public puppyRaffle;
​
constructor(address _puppyRaffle) {
puppyRaffle = PuppyRaffle(_puppyRaffle);
}
​
function withdrawFeeAttackByAddress() public {
puppyRaffle.withdrawFees();
}
​
receive() external payable {}
​
fallback() external payable {}
}

Test

function testAnyoneCanWithdrawFees() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
puppyRaffle.selectWinner();
//The Owner chose the feeAddress to be the attack address by mistake.
puppyRaffle.changeFeeAddress(address(attack));
attack.withdrawFeeAttackByAddress();
console.log("attack balance: %s", address(attack).balance); //8 ether
console.log("puupyRaffle balance: %s", address(puppyRaffle).balance); //0 ether
assertEq(address(puppyRaffle).balance, 0);
}

Tools Used

Foundry

Recommendations

Introduce the onlyOwner modifier or an equivalent access control mechanism to the withdrawFees function to restrict the withdrawal capability to only the contract owner.

- function withdrawFees() external {
+ function withdrawFees() external onlyOwner {
Updates

Lead Judging Commences

Hamiltonite Lead Judge almost 2 years 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.