First Flight #2: Puppy Raffle

Summary

This contract contains a vulnerability in its refund function that allows for a reentrancy attack which could lead to a significant loss of funds.

Vulnerability Details

OpenZeppelins Address library is used to send Ether back to the caller, but it does not protect against reentrancy.OpenZeppelin's sendValue() function forwards all remaining gas to the calling contract, so an attacker could implement a malicious fallback function in a contract they control, and it will be possible to recursively call refund before the state 'players[playerIndex] = address(0)' is updated.

Impact

This issue is very high impact and can lead to the loss of the entire balance of the smart contract.
An attacker can effectively drain all the funds from the smart contract, as well as lock the contract in such a way where 'selectWinner' will always revert,and the contract will be completely 'locked' meaning a new raffle can never start and no NFT's will be minted.

Additionally, other users who are not aware that the contract has been exploited could potentially still be able to enter the raffle, and have their funds drained even after the initial draining.

note: technically because of another bug in the duplicate check, an attacker will only be able to exploit this twice, before there are two zero addresses in the player array and the contract is locked from any new raffle entries.

PoC

'contract Attacker {
PuppyRaffle raffle;
uint256 index;

constructor(PuppyRaffle _raffle, uint256 _index) {
    raffle = _raffle;
    index = _index;
}

fallback() external payable {
    if (address(raffle).balance >= raffle.entranceFee()) {
        raffle.refund(index);
    }
}

function startAttack() external {
    raffle.refund(index);
}

}
'

Tools Used:

Manual Review

Recommendations

there are a few potential ways to approach this problem:

1. use 'Check Effects Interaction' pattern, so that the state change occurs before the ether is sent.

2. implement Openzeppelin's Reentrancy Guard.

3. use transfer() to limit the amount of gas forwarded, making it much more difficult to carry out the attack

4. utilize a push pull withdraw pattern to withdraw refunds.
   these solutions have different tradeoffs, but it is recommended to implement the Reentrancy Guard and at the same time use the Check Effects Interaction pattern.