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

Denial of Service

goat

The Denial of Service occurs on this line
https://github.com/CodeHawks-Contests/ai-puppy-raffle/blob/08e5b1fc6939b8da7792b2d13e43000c519d8897/src/PuppyRaffle.sol#L86

Denial of service occurs at looping through players array in enterRaffle function makes function hard to call later

Description

In enterRaffle function we loop through players[] array to check for duplicates which is bad practice due to denial of service.
Each time we looping through players array it will cost more gas and in enter raffle fucntion takes as parameter players array so the malicious user can enter 100 accounts at once and the 101th enter will cost a lot more gas to enter than the first few which is not good beacuse it give big adventage to users who enter faster.

function enterRaffle(address[] memory newPlayers) public payable { //aduit could be external for gas efficiency
//q what if someone enter 1000 players at once?
//audit consider make variable which set max players at one enter
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
//audit for gas efficeny storee newPlayers.length in a variable
for (uint256 i = 0; i < newPlayers.length; i++) {
players.push(newPlayers[i]);
}
// audit-high, DoS attack possible
// 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");
}
}
emit RaffleEnter(newPlayers);
}

Risk

Likelihood: HIGH
It will always occurs when many players enter the raffle what is our goal to make the protocol big

No special requiremenst to make this happen and attack require minimal cost.

Impact:HIGH

Complete Denial of Service - after a certain number of players (~200-300), no new players can enter due to block gas limit
Makes the protocol unusable and the early entrant funds are stuck in the protocol.

Proof of Concept

Here is the test for Dos in this function at first we enter and 100 playres to a raffle and console.log the totalGasCost for make 100 players to enter the raffle then we enter another 100 players store the total gas used in the variable totalGasUsed2 and at the end we compare through
subtractring these 2 variables how much the gas increased.

function testDos() public {
uint256 plaersNum = 100;
address[] memory players = new address[](plaersNum);
​
for(uint256 i = 0; i < plaersNum; i++){
players[i] = address(i);
}
uint256 gasStart = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * plaersNum}(players);
uint256 gasEnd = gasleft();
​
uint256 totalGasCost = gasStart - gasEnd;
​
console.log("Gas cost: ", totalGasCost);
​
uint256 playersNum2 = 100;
address[] memory players2 = new address[](playersNum2);
​
for(uint256 i = 0; i < playersNum2; i++){
players2[i] = address(i + plaersNum);
}
uint256 gasStart2 = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * playersNum2}(players2);
uint256 gasEnd2 = gasleft();
​
uint256 totalGasUsed2 = gasStart2 - gasEnd2;
​
console.log("Gas used for 200 players: ", totalGasUsed2);
console.log("Gas increase: ", totalGasUsed2 - totalGasCost);
}
​
​

.Recommended Mitigation
Here is the most recomended mitigation.Its just simple mapping for storing raffleId to an addresses
It works simple in the enterRaffle function in loop we just check if the player address equal to raffleId which equlas 1 at the start. them if its not equal we push player to array and store raffleId to this address, so if the player want to enter another time he has already stored raffleId = 1 to his addres so the require revert.

- remove this code
+mapping(address => uint256) public addressToRaffleId;
+uint256 public raffleId = 1;
​
function enterRaffle(address[] memory newPlayers) public payable { //aduit could be external for gas efficiency
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
for (uint256 i = 0; i < newPlayers.length; i++) {
+ require(addressToRaffleId[newPlayers[i]] != raffleId, "PuppyRaffle: Duplicate player");
players.push(newPlayers[i]);
+ addressToRaffleId[newPlayers[i]] = raffleId;
}
- 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 {
+ raffleId++;
delete players;
}
​
​
+ add this code
Updates

Lead Judging Commences

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