Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Refunded entries remain counted as active players, breaking raffle accounting and settlement

samuraiigintokii

1. Refunded entries remain counted as active players, breaking raffle accounting and settlement

Severity: High

Description

Normally, the players array tracks the active participants in the raffle, and its length is used to calculate the total prize pool and protocol fees when selecting a winner.

The issue occurs because the refund() function removes a player by setting their index in the array to address(0), but does not decrease the actual length of the players array. Consequently, the selectWinner() function continues to use the original, inflated array length to calculate the payouts, despite the contract holding less ETH after the refund.

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");
​
// Root cause: the slot is zeroed out, but the array length is not reduced
//@> players[playerIndex] = address(0);
​
emit RaffleRefunded(playerAddress);
(bool success,) = payable(msg.sender).call{value: entranceFee}("");
require(success, "PuppyRaffle: Failed to refund player");
}

Risk

Likelihood: High

  • Any legitimate player can request a refund at any time before the raffle ends.

  • A malicious actor can intentionally enter and refund to trigger the desync.

Impact: High

  • If the inflated calculated prizePool exceeds the actual contract balance, selectWinner() will revert when sending funds, causing a complete Denial of Service (DoS) and permanently locking all user funds in the contract.

  • If the contract has enough residual balance to pay the winner, the winner will receive more than 80% of the actual collected pool, stealing funds that were meant to back the protocol fees.

Proof of Concept

function testH1_ArrayDesync_LockedFunds() public {
// 1. Four players enter
address[] memory entrants = new address[](4);
entrants[0] = playerOne;
entrants[1] = playerTwo;
entrants[2] = playerThree;
entrants[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(entrants);
​
// 2. One player refunds. Contract balance drops, but array length stays 4.
uint256 indexOfPlayer = puppyRaffle.getActivePlayerIndex(playerTwo);
vm.prank(playerTwo);
puppyRaffle.refund(indexOfPlayer);
​
// 3. Time passes, raffle ends
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// 4. selectWinner reverts because calculated prizePool > actual balance
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
}

Recommended Mitigation

Remove the refunded player by swapping their position with the last element in the array and then popping the last element. This ensures the array length always matches the active, funded participants.

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);
+ players[playerIndex] = players[players.length - 1];
+ players.pop();
​
emit RaffleRefunded(playerAddress);
(bool success,) = payable(msg.sender).call{value: entranceFee}("");
require(success, "PuppyRaffle: Failed to refund player");
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 21 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-04] `PuppyRaffle::refund` replaces an index with address(0) which can cause the function `PuppyRaffle::selectWinner` to always revert

## Description `PuppyRaffle::refund` is supposed to refund a player and remove him from the current players. But instead, it replaces his index value with address(0) which is considered a valid value by solidity. This can cause a lot issues because the players array length is unchanged and address(0) is now considered a player. ## Vulnerability Details ```javascript players[playerIndex] = address(0); @> uint256 totalAmountCollected = players.length * entranceFee; (bool success,) = winner.call{value: prizePool}(""); require(success, "PuppyRaffle: Failed to send prize pool to winner"); _safeMint(winner, tokenId); ``` If a player refunds his position, the function `PuppyRaffle::selectWinner` will always revert. Because more than likely the following call will not work because the `prizePool` is based on a amount calculated by considering that that no player has refunded his position and exit the lottery. And it will try to send more tokens that what the contract has : ```javascript uint256 totalAmountCollected = players.length * entranceFee; uint256 prizePool = (totalAmountCollected * 80) / 100; (bool success,) = winner.call{value: prizePool}(""); require(success, "PuppyRaffle: Failed to send prize pool to winner"); ``` However, even if this calls passes for some reason (maby there are more native tokens that what the players have sent or because of the 80% ...). The call will thankfully still fail because of the following line is minting to the zero address is not allowed. ```javascript _safeMint(winner, tokenId); ``` ## Impact The lottery is stoped, any call to the function `PuppyRaffle::selectWinner`will revert. There is no actual loss of funds for users as they can always refund and get their tokens back. However, the protocol is shut down and will lose all it's customers. A core functionality is exposed. Impact is high ### Proof of concept To execute this test : forge test --mt testWinnerSelectionRevertsAfterExit -vvvv ```javascript function testWinnerSelectionRevertsAfterExit() public playersEntered { vm.warp(block.timestamp + duration + 1); vm.roll(block.number + 1); // There are four winners. Winner is last slot vm.prank(playerFour); puppyRaffle.refund(3); // reverts because out of Funds vm.expectRevert(); puppyRaffle.selectWinner(); vm.deal(address(puppyRaffle), 10 ether); vm.expectRevert("ERC721: mint to the zero address"); puppyRaffle.selectWinner(); } ``` ## Recommendations Delete the player index that has refunded. ```diff - players[playerIndex] = address(0); + players[playerIndex] = players[players.length - 1]; + players.pop() ```

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!