Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View all submissions
Previous Next
Submission Details
Impact: high
Likelihood: high

Refunded players are still counted in prize/fee accounting, bricking winner selection and fee withdrawal

Author Revealed upon completion

Root + Impact

Description

refund() blanks the player's slot (players[playerIndex] = address(0)) but selectWinner() computes the pot from players.length, which still counts refunded slots as paying players. The prize pool and the fee are therefore inflated by players who already took their money back.

// refund() leaves an address(0) hole in the array:
@> players[playerIndex] = address(0);
​
// ...but selectWinner() still counts it as a paying player:
@> uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;

Risk

Likelihood: High — refunding is a documented feature, so this triggers in normal use; no attacker is needed.

Impact: High — the raffle breaks in two ways. First, selectWinner() reverts when refunded players are more than 25% of active players: the prize pool (80% of players.length) exceeds the real balance, so the winner payment fails. In a minimum 4-player raffle, a single refund bricks winner selection (prizePool 3.2 ETH vs 3 ETH balance). If the winner index lands on a refunded (address(0)) slot, the prize is sent to address(0) and _safeMint reverts. Second, withdrawFees() is permanently broken after ANY refund: the fee is inflated by refunded players, so totalFees never equals the real balance and the balance check can never pass — protocol fees stuck forever.

Proof of Concept

The first test shows one refund bricking winner selection in a 4-player raffle: after the refund the balance is 3 ETH but the prize pool is 3.2 ETH, so selectWinner() reverts with "Failed to send prize pool to winner". The second test shows the fee side: 8 players, 1 refund, selectWinner() succeeds, but the balance is 0.6 ETH vs totalFees 1.6 ETH, so withdrawFees() reverts forever.

function test_M2_OneRefundBricksSelectWinner() public {
address[] memory players = _enterPlayers(4, 100);
vm.prank(players[3]);
raffle.refund(3); // balance now 3 ETH, but players.length still 4
assertEq(address(raffle).balance, 3 ether);
​
vm.warp(block.timestamp + duration + 1);
address caller = _findCaller(4, 2); // winner must be a real player (index 0-2)
vm.prank(caller);
vm.expectRevert(bytes("PuppyRaffle: Failed to send prize pool to winner"));
raffle.selectWinner(); // prizePool = 3.2 ETH > 3 ETH balance
}
​
function test_M2_RefundBreaksWithdrawFees() public {
address[] memory players = _enterPlayers(8, 100);
vm.prank(players[7]);
raffle.refund(7); // balance 7 ETH
vm.warp(block.timestamp + duration + 1);
address caller = _findCaller(8, 6); // winner must be a real player (index 0-6)
vm.prank(caller);
raffle.selectWinner(); // succeeds: prizePool 6.4 ETH < 7 ETH balance
​
assertEq(address(raffle).balance, 0.6 ether, "only 0.6 ETH left, not the 1.6 ETH fee");
assertEq(uint256(raffle.totalFees()), 1.6 ether, "fee accounting still counts the refunded player");
vm.expectRevert(bytes("PuppyRaffle: There are currently players active!"));
raffle.withdrawFees(); // 0.6 != 1.6 -> fees stuck forever
}

Recommended Mitigation

Track the number of active players (or the real collected amount) separately from players.length, and compute the prize and fee from active players only. Alternatively, remove refunded players from the array (swap-and-pop) instead of leaving address(0) holes.

+ uint256 public activePlayerCount;
...
- uint256 totalAmountCollected = players.length * entranceFee;
+ uint256 totalAmountCollected = activePlayerCount * entranceFee;

Support

FAQs

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

Give us feedback!