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

Gas inefficiencies (O(n) lookups)

lordbolton

Root + Impact

Description

The PuppyRaffle::getActivePlayerIndex function uses a linear search (O(n) complexity) to find a player's index in the array. This becomes increasingly expensive as more players join the raffle. While view functions don't cost gas when called externally, users must call this function to get their index before calling refund(), resulting in poor user experience and unnecessary complexity.

/// @param player the address of a player in the raffle
/// @return the index of the player in the array, if they are not active, it returns 0
function getActivePlayerIndex(address player) external view returns (uint256) {
// @> O(n) linear search - loops through entire array
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player) {
return i;
}
}
return 0;
}
​
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
// ...
}

The Problem:

  1. Players must call getActivePlayerIndex() off-chain to get their index

  2. Then call refund(index) in a separate transaction

  3. With 1,000 players, finding index requires checking up to 1,000 slots

  4. Poor UX: two-step process for a simple refund

  5. Gas inefficient: unnecessary external call pattern

Risk

Likelihood: High - Every player who wants a refund must use this pattern.

Impact: Low - Increases gas costs for users and complexity, but doesn't break functionality or cause fund loss.

Proof of Concept

Gas Cost Analysis:

// With 100 players:
getActivePlayerIndex(player99) // Checks ~100 slots
// Gas: ~2,000 gas per check = ~200,000 gas (if called on-chain)
​
// With 1,000 players:
getActivePlayerIndex(player999) // Checks ~1,000 slots
// Gas: ~2,000 gas per check = ~2,000,000 gas (if called on-chain)

User Experience Issue:

// Current workflow (2 transactions):
// Step 1: Get index (off-chain view call)
const myIndex = await puppyRaffle.getActivePlayerIndex(myAddress);
​
// Step 2: Refund using index (on-chain transaction)
await puppyRaffle.refund(myIndex);
​
// Problem: If another player enters/exits between steps, index becomes stale

Alternative Issue - Index Can Return Wrong Value:

The function returns 0 for non-existent players, but 0 is also a valid index for the first player:

// Player at index 0 exists
players[0] = 0xAlice;
​
// Non-existent player
getActivePlayerIndex(0xBob); // Returns 0 (but Bob isn't at index 0!)
​
// Cannot distinguish between:
// 1. Player at index 0
// 2. Player not in raffle

Tools Used

Manual review

Recommended Mitigation

Use a mapping to track player indices in O(1) constant time:

+ mapping(address => uint256) public addressToPlayerIndex;
​
function enterRaffle(address[] memory newPlayers) public payable {
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
for (uint256 i = 0; i < newPlayers.length; i++) {
players.push(newPlayers[i]);
+ addressToPlayerIndex[newPlayers[i]] = players.length - 1;
}
// ... duplicate check ...
emit RaffleEnter(newPlayers);
}
​
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);
+ addressToPlayerIndex[msg.sender] = 0;
emit RaffleRefunded(playerAddress);
}

Even Better - Remove Index Parameter:

- function refund(uint256 playerIndex) public {
+ function refund() public {
+ uint256 playerIndex = addressToPlayerIndex[msg.sender];
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);
+ addressToPlayerIndex[msg.sender] = 0;
emit RaffleRefunded(playerAddress);
}
​
- function getActivePlayerIndex(address player) external view returns (uint256) {
- for (uint256 i = 0; i < players.length; i++) {
- if (players[i] == player) {
- return i;
- }
- }
- return 0;
- }

Benefits:

  • ✅ O(1) lookup instead of O(n)

  • ✅ Better UX: single transaction for refund

  • ✅ No need for external index lookup

  • ✅ Eliminates stale index issues

  • ✅ Gas savings for users

Note: This is a quality-of-life improvement. While it doesn't fix critical vulnerabilities, it significantly improves user experience and gas efficiency.

Updates

Lead Judging Commences

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

[M-01] `PuppyRaffle: enterRaffle` Use of gas extensive duplicate check leads to Denial of Service, making subsequent participants to spend much more gas than prev ones to enter

## Description `enterRaffle` function uses gas inefficient duplicate check that causes leads to Denial of Service, making subsequent participants to spend much more gas than previous users to enter. ## Vulnerability Details In the `enterRaffle` function, to check duplicates, it loops through the `players` array. As the `player` array grows, it will make more checks, which leads the later user to pay more gas than the earlier one. More users in the Raffle, more checks a user have to make leads to pay more gas. ## Impact As the arrays grows significantly over time, it will make the function unusable due to block gas limit. This is not a fair approach and lead to bad user experience. ## POC In existing test suit, add this test to see the difference b/w gas for users. once added run `forge test --match-test testEnterRaffleIsGasInefficient -vvvvv` in terminal. you will be able to see logs in terminal. ```solidity function testEnterRaffleIsGasInefficient() public { vm.startPrank(owner); vm.txGasPrice(1); /// First we enter 100 participants uint256 firstBatch = 100; address[] memory firstBatchPlayers = new address[](firstBatch); for(uint256 i = 0; i < firstBatchPlayers; i++) { firstBatch[i] = address(i); } uint256 gasStart = gasleft(); puppyRaffle.enterRaffle{value: entranceFee * firstBatch}(firstBatchPlayers); uint256 gasEnd = gasleft(); uint256 gasUsedForFirstBatch = (gasStart - gasEnd) * txPrice; console.log("Gas cost of the first 100 partipants is:", gasUsedForFirstBatch); /// Now we enter 100 more participants uint256 secondBatch = 200; address[] memory secondBatchPlayers = new address[](secondBatch); for(uint256 i = 100; i < secondBatchPlayers; i++) { secondBatch[i] = address(i); } gasStart = gasleft(); puppyRaffle.enterRaffle{value: entranceFee * secondBatch}(secondBatchPlayers); gasEnd = gasleft(); uint256 gasUsedForSecondBatch = (gasStart - gasEnd) * txPrice; console.log("Gas cost of the next 100 participant is:", gasUsedForSecondBatch); vm.stopPrank(owner); } ``` ## Recommendations Here are some of recommendations, any one of that can be used to mitigate this risk. 1. User a mapping to check duplicates. For this approach you to declare a variable `uint256 raffleID`, that way each raffle will have unique id. Add a mapping from player address to raffle id to keep of users for particular round. ```diff + uint256 public raffleID; + mapping (address => uint256) public usersToRaffleId; . . function enterRaffle(address[] memory newPlayers) public payable { require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle"); for (uint256 i = 0; i < newPlayers.length; i++) { players.push(newPlayers[i]); + usersToRaffleId[newPlayers[i]] = true; } // Check for duplicates + for (uint256 i = 0; i < newPlayers.length; i++){ + require(usersToRaffleId[i] != raffleID, "PuppyRaffle: Already a participant"); - for (uint256 i = 0; i < players.length - 1; i++) { - for (uint256 j = i + 1; j < players.length; j++) { - require(players[i] != players[j], "PuppyRaffle: Duplicate player"); - } } emit RaffleEnter(newPlayers); } . . . function selectWinner() external { //Existing code + raffleID = raffleID + 1; } ``` 2. Allow duplicates participants, As technically you can't stop people participants more than once. As players can use new address to enter. ```solidity function enterRaffle(address[] memory newPlayers) public payable { require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle"); for (uint256 i = 0; i < newPlayers.length; i++) { players.push(newPlayers[i]); } emit RaffleEnter(newPlayers); } ```

Support

FAQs

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

Give us feedback!