Potential Vulnerability in the PuppyRaffle Contract – Addressing Refund & Winner Selection Edge Case

[mosesmrima]

Hello everyone,

I wanted to share a potential vulnerability I identified in the PuppyRaffle contract. I am aware Patrick mentions that not all issues will be addressed during the course, allow me to share my excitement though :) .

Description of the Issue

When an active player calls the PuppyRaffle:refund function, they are refunded, and their address in the players array is set to address(0). However, when the PuppyRaffle:selectWinner function is called, if the randomly chosen winner index corresponds to an exited player (address(0)), the prize pool is sent to the zero address, effectively burning the funds.

Proof of Concept

The following test demonstrates the issue:

function testRefundAndWin() public {
    address[] memory players = new address[](4);
    for (uint256 i = 0; i < players.length; ++i) {
        players[i] = address(i + 1);
    }
    puppyRaffle.enterRaffle{value: players.length * entranceFee}(players);
    
    vm.prank(players[1]);
    puppyRaffle.refund(1); // Player exits and their address becomes address(0)
    
    vm.warp(duration * 2);
    vm.roll(duration * 2);
    
    puppyRaffle.selectWinner(); // Risk: Winner may be chosen as address(0)
}

Recommended Mitigation

To avoid selecting a refunded player, the selectWinner function can be updated to skip indices that reference address(0) by implementing a loop:

uint256 winnerIndex = uint256(
    keccak256(
        abi.encodePacked(msg.sender, block.timestamp, block.difficulty)
    )
) % players.length;

while (players[winnerIndex] == address(0)) {
    winnerIndex = (winnerIndex + 1) % players.length;
}

address winner = players[winnerIndex];

This ensures that only valid, active players are considered for winner selection.

---

Let's keep grinding, WAGMI!

[EngrPips]

What a catch, Keep grinding.

[Blockchain-Oracle]

interesting, was about to create a discussion on this.
You mind to discuss what other errors @PatrickAlphaC must have missed ?

[mosesmrima]

Hey sure thing! Have you uncovered more issues?

[EngrPips]

Interesting.

[PatrickAlphaC]

LFG! I think we covered this in the original report? It's been a while I'd have to double check 😅

[EngrPips]

indeed it been a long time.