- High: 3
- Medium: 2
- Low: 1
https://github.com/Cyfrin/2023-10-Puppy-Raffle/blob/main/src/PuppyRaffle.sol#L96-#L104
The refund function can be exploited by a Reentrancy attack and drain all the ETH from the Raffle.
The refund function resets the players[playerIndex] AFTER it sends ether to the msg.sender. This opens up a reentrancy attack vector. Assuming msg.sender a smart contract and has entered the raffle and knows it's index within the players array. The contract can use a recieve function that calls refund it is allowed to reenter the function and receive extra ether. They can repeat this until they have drain the raffle of all ether.
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
payable(msg.sender).sendValue(entranceFee);
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}Loss of all ether locked in the PuppyRaffle contract
Manual Review
Use the Checks, Effects, Interactions flow with functions that send ether. For this function move the players[playerIndex] = address(0); before the line payable(msg.sender).sendValue(entranceFee);
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
+ players[playerIndex] = address(0);
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}https://github.com/Cyfrin/2023-10-Puppy-Raffle/blob/main/src/PuppyRaffle.sol#L103
NFTs and ETH can be burned if player calls refund
When a player calls refund the address at their index in the players array is set to address(0). If the winner index happens to be address(0) the winning funds and NFT will be burned and lost forever
Loss of NFTs and ETH
Manual Review
When deleting something from an array it is best to shift the index you want to delete to the end of the array and use .pop to remove it.
https://github.com/Cyfrin/2023-10-Puppy-Raffle/blob/main/src/PuppyRaffle.sol#L96-#L98
The refund function sends the entranceFee to the player at the index of the array and if they are the message sender. However that player may not be the one who bought in.
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
payable(msg.sender).sendValue(entranceFee);
To enter a raffle an array of addresses is given and the entry fee is paid for by the msg.sender not necessarly by the player who calls for the refund. A player who did not pay for the entranceFee can calim a refund and basically steal the funds from the raffle and will likey hurt the feelings of the player who bought them their entrance to the raffle. On the flip side, someone who paid for multiple entries will only get a partial refund from calling this function.
Loss of funds from the raffle pool and hurt feelings.
Manual Review
Track the address that paid for a player's entrance and refund that player if a player wishes to leave the raffle.
https://github.com/Cyfrin/2023-10-Puppy-Raffle/blob/main/src/PuppyRaffle.sol#L79-#L89
The enterRaffle function itterates over arrays three times and will revert if they become too long.
Looping through arrays is gas intensive. The enterRaffle function does this three times. Once over the array passed by the user and twice over the player array to check for duplicates.
function enterRaffle(address[] memory newPlayers) public payable {
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
for (uint256 i = 0; i < newPlayers.length; i++) {
players.push(newPlayers[i]);
}
// Check for duplicates
for (uint256 i = 0; i < players.length - 1; i++) {
for (uint256 j = i + 1; j < players.length; j++) {
require(players[i] != players[j], "PuppyRaffle: Duplicate player");
}
}If either of these arrays is or becomes too large it will run out of gas. If a user passed an array of address who wishes to enter and it is too long they will have to split it into multiple function calls. But then when the players array becomes too long no new players will be able to enter the raffle
Manual Review
Only allow players to enter themselves into the raffle and track their entry with a bool. This would also disallow users from registering unwilling accounts into the raffle.
https://github.com/Cyfrin/2023-10-Puppy-Raffle/blob/main/src/PuppyRaffle.sol#L2
Puppy Raffle uses an outdated Solidity pragma which can lean to mathmatical errors
The Puppy Raffle contract uses pragma solidity ^0.7.6; whihch is outdated.
The major issue is that this version of Solidity does not check for arithmatic over/underflows at the language level and can cause issues with mathmatical operations
Manual Review
Use at least pragma 0.8.0 but it is best to use a version closer to the most recently updated
https://github.com/Cyfrin/2023-10-Puppy-Raffle/blob/main/src/PuppyRaffle.sol#L2
The Puppy Raffle contract uses a floating pragma
Using floating pragmas can lead to damaged or non fuctional contracts
Floating pragmas allow for potential inconsistencies and vulnerabilities due to differences between Solidity compiler versions. It is possible these contracts get deployed with an outdated compiler version, or on a diffrent chain, which could introduce bugs that negatively affect the Puppy Raffle.
Manual Review
Fix the solidity pragma to what was ued in testing the contract.