Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Strict equality on the contract balance in `PuppyRaffle::withdrawFees` allows anyone to permanently block the function by force-sending ETH

smilenot

[M-3] Strict equality on the contract balance in PuppyRaffle::withdrawFees allows anyone to permanently block the function by force-sending ETH

Description: PuppyRaffle::withdrawFees function uses a strict equality between address(this).balance and uint256(totalFees). Any user (whether malicious or not) can force-send ETH to the contract breaking this strict equality and permanently blocking the function. Sending more ETH cannot fix the block: address(this).balance can only increase, so once it exceeds totalFees the equality can never hold again.

function withdrawFees() external {
// @audit Mishandling ETH
@> require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

Impact: The contract remains without any possibility to withdraw the fees. The rest of the contract functionality will still work but accumulated fees will remain permanently blocked in the contract.

Proof of Concept: In the following test we can see how, after an external contract fires selfdestruct (which resides in SelfDestructiveContract::destroy function) with PuppyRaffle address as the receiver, the PuppyRaffle::withdrawFees function fires the revert from its first require causing a permanent block.

PoC

Place the following test into PuppyRaffle.t.sol.

function test_StrictEqualityBlocks() public playersEntered {
SelfDestructiveContract selfDestructiveContract = new SelfDestructiveContract(puppyRaffle);
vm.deal(address(selfDestructiveContract), 1 wei);
vm.warp(puppyRaffle.raffleStartTime() + puppyRaffle.raffleDuration());
vm.roll(block.number + 1);
​
selfDestructiveContract.destroy();
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
}
​
...
​
// This goes outside the PuppyRaffleTest contract
contract SelfDestructiveContract {
PuppyRaffle puppyRaffle;
​
constructor(PuppyRaffle _puppyRaffle) {
puppyRaffle = _puppyRaffle;
}
​
function destroy() external {
selfdestruct(payable(address(puppyRaffle)));
}
}

Note that since EIP-6780 (Cancun), selfdestruct no longer deletes the contract unless called in the same transaction as its creation, but it still forwards the balance, which is all that is needed here.

Recommended Mitigation: There are a few recommendations:

  1. Consider changing the strict equality == to checking the players length which correctly enforces that no active player remains in the raffle and removes the ETH mishandling issue.

function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(players.length == 0, "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;

This solves the ETH mishandling but the force-sent ETH will still be blocked in the contract because totalFees variable does not account for it. If this ETH needs to be withdrawn there are two main changes to apply:

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

Note: In the code above, totalFees variable is removed as it is not needed anymore, we use address(this).balance instead.

or

function removeExceededETH() external onlyOwner {
uint256 exceededETH = address(this).balance - uint256(totalFees);
require(exceededETH != 0, "PuppyRaffle: There isn't any exceeded ETH");
​
(bool success,) = feeAddress.call{value: exceededETH}("");
require(success, "PuppyRaffle: Failed to withdraw exceeded ETH");
}

The code above should be placed in PuppyRaffle contract to withdraw the exceeded fees.

The last snippet mitigates the bug without changing PuppyRaffle::withdrawFees function. PuppyRaffle::withdrawFees will still get blocked but firing PuppyRaffle::removeExceededETH unlocks it as the exceeded ETH is removed. The downside of this solution is that the block is still possible and every time it happens PuppyRaffle::removeExceededETH needs to be called.

As a conclusion, address(this).balance should never be used in a strict equality. A contract's balance can always be increased by external parties without executing any of its code. This might break some contract functionality as in the PuppyRaffle::withdrawFees case.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[M-02] Slightly increasing puppyraffle's contract balance will render `withdrawFees` function useless

## Description An attacker can slightly change the eth balance of the contract to break the `withdrawFees` function. ## Vulnerability Details The withdraw function contains the following check: ``` require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!"); ``` Using `address(this).balance` in this way invites attackers to modify said balance in order to make this check fail. This can be easily done as follows: Add this contract above `PuppyRaffleTest`: ``` contract Kill { constructor (address target) payable { address payable _target = payable(target); selfdestruct(_target); } } ``` Modify `setUp` as follows: ``` function setUp() public { puppyRaffle = new PuppyRaffle( entranceFee, feeAddress, duration ); address mAlice = makeAddr("mAlice"); vm.deal(mAlice, 1 ether); vm.startPrank(mAlice); Kill kill = new Kill{value: 0.01 ether}(address(puppyRaffle)); vm.stopPrank(); } ``` Now run `testWithdrawFees()` - ` forge test --mt testWithdrawFees` to get: ``` Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest [FAIL. Reason: PuppyRaffle: There are currently players active!] testWithdrawFees() (gas: 361718) Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.40ms ``` Any small amount sent over by a self destructing contract will make `withdrawFees` function unusable, leaving no other way of taking the fees out of the contract. ## Impact All fees that weren't withdrawn and all future fees are stuck in the contract. ## Recommendations Avoid using `address(this).balance` in this way as it can easily be changed by an attacker. Properly track the `totalFees` and withdraw it. ```diff 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"); } ```

Support

FAQs

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

Give us feedback!