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

Active Players Count Check Doesn't Verify Non-Zero Addresses

muzammilmohammed678

Root + Impact

Description

  • * The `selectWinner()` function checks `players.length >= 4` to ensure there are enough players, but doesn't verify that there are actually 4 active (non-zero) players.

    * When players refund, their slots are set to `address(0)` but remain in the array. The length check passes even if all players have refunded, leading to selection of `address(0)` as the winner.

    ```solidity:125:130:src/PuppyRaffle.sol

    function selectWinner() external {

    require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");

    require(players.length >= 4, "PuppyRaffle: Need at least 4 players");

    uint256 winnerIndex =

    uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;

    address winner = players[winnerIndex];

    ```


Risk

Likelihood:

  • * This occurs whenever players have refunded before `selectWinner()` is called

    * Refunds are allowed until the raffle ends, making this scenario common

    * The check `players.length >= 4` passes even when all entries are `address(0)`

Impact:

  • * Winner can be `address(0)`, causing prize pool to be burned

    * NFT minted to `address(0)` is effectively burned

    * The check is misleading - it says "need 4 players" but might only have refunded slots

    * Funds are permanently lost

Proof of Concept

```solidity
// 4 players enter
address[] memory players = new address[](4);
players[0] = alice;
players[1] = bob;
players[2] = charlie;
players[3] = dave;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
// All players refund
puppyRaffle.refund(0); // alice
puppyRaffle.refund(1); // bob
puppyRaffle.refund(2); // charlie
puppyRaffle.refund(3); // dave
// Array: [address(0), address(0), address(0), address(0)]
// players.length = 4, but all are address(0)
​
// selectWinner() is called
// Check passes: players.length >= 4 ✓
// winnerIndex could be 0, 1, 2, or 3
// winner = address(0) - funds burned!
```

Recommended Mitigation

```diff
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
- require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
+
+ // Count only active (non-zero) players
+ uint256 activePlayers = 0;
+ address[] memory activePlayersList = new address[](players.length);
+ uint256 activeIndex = 0;
+ for (uint256 i = 0; i < players.length; i++) {
+ if (players[i] != address(0)) {
+ activePlayers++;
+ activePlayersList[activeIndex] = players[i];
+ activeIndex++;
+ }
+ }
+ require(activePlayers >= 4, "PuppyRaffle: Need at least 4 active players");
+
uint256 winnerIndex =
- uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
- address winner = players[winnerIndex];
+ uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % activePlayers;
+ address winner = activePlayersList[winnerIndex];
// ... rest of function
}
```
​
This ensures only active players can be selected as winners and prevents funds from being burned.
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 16 days ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-01] Potential Loss of Funds During Prize Pool Distribution

## Description In the `selectWinner` function, when a player has refunded and their address is replaced with address(0), the prize money may be sent to address(0), resulting in fund loss. ## Vulnerability Details In the `refund` function if a user wants to refund his money then he will be given his money back and his address in the array will be replaced with `address(0)`. So lets say `Alice` entered in the raffle and later decided to refund her money then her address in the `player` array will be replaced with `address(0)`. And lets consider that her index in the array is `7th` so currently there is `address(0)` at `7th index`, so when `selectWinner` function will be called there isn't any kind of check that this 7th index can't be the winner so if this `7th` index will be declared as winner then all the prize will be sent to him which will actually lost as it will be sent to `address(0)` ## Impact Loss of funds if they are sent to address(0), posing a financial risk. ## Recommendations Implement additional checks in the `selectWinner` function to ensure that prize money is not sent to `address(0)`

Support

FAQs

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

Give us feedback!