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

Looping through players array to check for duplicates in PuppyRaffle::enterRaffle is a potential DoS vector, incrementing gas costs for future entrants

maxwellwire17

Root + Impact

Description

  • Description: The PuppyRaffle::enterRaffle function loops through the players array to check for duplicates. However, the longer the PuppyRaffle:players array is, the more checks a new player will have to make. This means that the gas costs for players who enter right when the raffle starts will be dramatically lower than those who enter later. Every additional address in the players array, is an additional check the loop will have to make.

// Root cause in the codebase with @> marks to highlight the relevant section
// Check for duplicates
@> 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");
}
}

Risk

Likelihood:

  • Every time a new player calls PuppyRaffle::enterRaffle, the function’s for loop iterates over the entire existing players array to check for duplicates. This occurs on every successful entry, not just under specific conditions.

  • As the raffle continues and more players enter, the players array grows indefinitely. A player entering when the array has 100 addresses will experience 100 loop iterations, whereas a player entering when the array has 1,000 addresses will experience 1,000 iterations.

Impact:

  • Late entrants face significantly higher gas costs than early entrants. This creates an unfair economic advantage/disadvantage among users, potentially discouraging participation as the raffle fills up.

  • A malicious user could front-run a legitimate transaction by rapidly adding many dummy addresses to the players array, artificially inflating the gas cost for the victim. The victim’s transaction may then fail due to out-of-gas errors or become prohibitively expensive to execute.

Proof of Concept

If we have 2 sets of 100 players enter, the gas costs will be as such:

  • 1st 100 players: 6252039

  • 2nd 100 players: 18067741

This is more than 3x as expensive for the second set of 100 players!

This is due to the for loop in the PuppyRaffle::enterRaffle function.

function testReadDuplicateGasCosts() public {
vm.txGasPrice(1);
​
// We will enter 5 players into the raffle
uint256 playersNum = 100;
address[] memory players = new address[](playersNum);
for (uint256 i = 0; i < playersNum; i++) {
players[i] = address(i);
}
// And see how much gas it cost to enter
uint256 gasStart = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * playersNum}(players);
uint256 gasEnd = gasleft();
uint256 gasUsedFirst = (gasStart - gasEnd) * tx.gasprice;
console.log("Gas cost of the 1st 100 players:", gasUsedFirst);
​
// We will enter 5 more players into the raffle
for (uint256 i = 0; i < playersNum; i++) {
players[i] = address(i + playersNum);
}
// And see how much more expensive it is
gasStart = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * playersNum}(players);
gasEnd = gasleft();
uint256 gasUsedSecond = (gasStart - gasEnd) * tx.gasprice;
console.log("Gas cost of the 2nd 100 players:", gasUsedSecond);
​
assert(gasUsedFirst < gasUsedSecond);
// Logs:
// Gas cost of the 1st 100 players: 6252039
// Gas cost of the 2nd 100 players: 18067741
}

Recommended Mitigation

  1. Consider allowing duplicates. Users can make new wallet addresses anyways, so a duplicate check doesn't prevent the same person from entering multiple times, only the same wallet address.

  2. Consider using a mapping to check duplicates. This would allow you to check for duplicates in constant time, rather than linear time. You could have each raffle have a uint256 id, and the mapping would be a player address mapped to the raffle Id.

+ mapping(address => uint256) public addressToRaffleId;
+ uint256 public raffleId = 0;
.
.
.
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]);
+ addressToRaffleId[newPlayers[i]] = raffleId;
}
​
- // Check for duplicates
+ // Check for duplicates only from the new players
+ for (uint256 i = 0; i < newPlayers.length; i++) {
+ require(addressToRaffleId[newPlayers[i]] != raffleId, "PuppyRaffle: Duplicate player");
+ }
- for (uint256 i = 0; i < players.length; i++) {
- for (uint256 j = i + 1; j < players.length; j++) {
- require(players[i] != players[j], "PuppyRaffle: Duplicate player");
- }
- }
emit RaffleEnter(newPlayers);
}
.
.
.
function selectWinner() external {
+ raffleId = raffleId + 1;
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 10 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!