Summary

Function refund() is open to a reentrancy exploit because it sends ether before resetting the players array.

Vulnerability Details

The function refund() allows a player to withdraw their entrance fee from the raffle contract. However, the function does not follow the checks-effects-interactions pattern, which is a best practice to prevent reentrancy attacks. The function first sends ether to the player using sendValue(), which can trigger a fallback function on the recipient contract. If the recipient contract is malicious, it can call refund() again before the original call is finished, and withdraw more ether than it should. This can drain the raffle contract's balance and affect other players. The function does not update the players array until after the ether transfer, which means that the same player can be refunded multiple times.

Impact

This exploit will cause loss of funds for the raffle contract and other players entrance fees. All ether can be drained.

Tools Used

Manual code review

Recommendations

To fix this vulnerability:

• Update the state before sending ether: Move the line players[playerIndex] = address(0); before the line payable(msg.sender).sendValue(entranceFee);. This will prevent the same player from being refunded multiple times.

• Use transfer() instead of sendValue(): Replace the line payable(msg.sender).sendValue(entranceFee); with payable(msg.sender).transfer(entranceFee);. This will limit the gas available for the recipient contract's fallback function and make it harder to perform a reentrancy attack.

• Use a reentrancy guard: Add a modifier that prevents nested calls to the refund() function. For example, you can use OpenZeppelin's ReentrancyGuard.

Proof of Concept (PoC)

Here is a PoC contract that demonstrates how an attacker could exploit this vulnerability:

This contract first enters into a raffle game by calling enterRaffle(). Then it triggers a reentrancy attack by calling attack(), which calls refund() on PuppyRaffle. The refund is received in its fallback function, which immediately calls refund() again if there are still funds in PuppyRaffle. and this way the contract gets drained.