First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Denial due of service due to looping over an unbounded `players` array in `PuppyRaffle`

ciara

The PuppyRaffle::enterRaffle(), PuppyRaffle::getActivePlayerId, and PuppyRaffle::_isActivePlayer() methods loop over the players array. The players array does not have an upper bound on its length. Each time a player joins the raffle by calling enterRaffle(), the array is looped over twice to check for duplicates before adding the player(s) to the array. As the number of players increases, the gas cost for entering, getting the player's ID, and whether they are active becomes more costly and can result in a gas cost that surpasses the block gas limit. This will mean that the transaction will revert leaving the contract's core functionalities (entering the raffle, getting the player ID, and whether they are active) in a state of denial of service (DoS).

Vulnerability Details

In enterRaffle() on line 86-90, there are two loops over the players[] array when checking that the addresses are not duplicated:

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]);
}
// 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);
}

In getActivePlayer() on line 111-115, the players array is looped over to check if the address provided as argument matches any of the addresses in the players array and returns the index of the player if it does:

function getActivePlayerIndex(address player) external view returns (uint256) {
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player) {
return i;
}
}
return 0;
}

In _isActivePlayer, on line 173-180 the players array is looped over to check if the caller address matches any of the addresses in the players array:

function _isActivePlayer() internal view returns (bool) {
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == msg.sender) {
return true;
}
}
return false;
}

All these functions loop over the players array. As there are no restrictions on the number of players in the lottery, if the players array surpasses a certain threshold length, the gas limit will be breached leading to a state of DoS for the current lottery.

Impact

When the players array increases past the threshold length, there will be a DoS when calling the following functions:

  • enterRaffle()

  • getActivePlayerIndex()

  • _isActivePlayer()

Prospective players will not be able to enter the raffle or retrieve their index, required to be able to call refund(). This is a core functionality of the contract but this state of DoS will only exist for the duration of a single raffle, therefore this is a medium-severity vulnerability.

Proof of Concept

Working Test Case

The following test demonstrates that as the number of players increases, the gas cost to call enterRaffle() to enter a single player into the lottery increases.

function test_poc_gasCostPositivelyCorreclatedWithNumberOfPlayers() public {
vm.txGasPrice(2);
​
// Gas Cost to enter one player
uint256 gasStart = gasleft();
​
address[] memory players = new address[](1);
players[0] = playerOne;
puppyRaffle.enterRaffle{value: entranceFee}(players);
​
uint256 gasEnd = gasleft();
uint256 gasUsed = (gasStart - gasEnd) * tx.gasprice;
console.log("Gas used to enter the first player: ", gasUsed);
​
// many new players enter
address[] memory newPlayers = new address[](7);
newPlayers[0] = playerTwo;
newPlayers[1] = playerThree;
newPlayers[2] = makeAddr("patrick");
newPlayers[3] = makeAddr("gio");
newPlayers[4] = makeAddr("alex");
newPlayers[5] = makeAddr("ciara");
newPlayers[6] = makeAddr("vitto");
puppyRaffle.enterRaffle{value: entranceFee * 7}(newPlayers);
​
// Gas Cost to enter as eighth player
uint256 gasStartMany = gasleft();
​
address[] memory lastPlayer = new address[](1);
lastPlayer[0] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee}(lastPlayer);
​
uint256 gasEndMany = gasleft();
uint256 gasUsedMany = (gasStartMany - gasEndMany) * tx.gasprice;
console.log("Gas used to the enter eighth player: ", gasUsedMany);
​
assertGe(gasUsedMany, gasUsed);
}

This test passes if the cost for the eighth player is higher than for the first player.

Running this test yields the following output:

$ forge test --mt test_poc_gasCostPositivelyCorreclatedWithNumberOfPlayers -vvv
​
// output
[PASS] test_poc_gasCostPositivelyCorreclatedWithNumberOfPlayers() (gas: 344944)
Logs:
Gas used to enter the first player: 126616
Gas used to enter the eighth player: 131240
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 16.96ms

As observed above, the gas cost for the first player to enter was 126616 whereas the gas cost for the eighth player was 131240.

As the number of players increases, the gas cost will increase and eventually surpass the gas limit resulting in a state of DoS as expected.

Recommended Mitigation

Removing unbounded array issues can be achieved in a few ways:

  • Limit the number of players who can join a raffle.

  • Modify the logic to avoid looping over unbounded arrays.

    • One way to achieve this is by storing a mapping of address to bool to determine if a player is active or not. _isActivePlayer() can be removed and enterRaffle() modified:

+ mapping(address->bool) activityStatus;
​
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(activityStatus(newPlayers[i]) == false, "Player already exists)
activityStatus[newPlayers[i]] = true;
players.push(newPlayers[i]);
}
​
- 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);
}

To refund(), the activityStatus will be set to false for the refunding address.

Since getActivePlayerIndex() is only used externally so that the returned value can be used to call refund(), getActivePlayerIndex() can be removed and refund() modified to take an address:

+ function refund(address playeraddress) public {
- address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
+ require(activityStatus[playerAddress] == true, "PuppyRaffle: Player already refunded, or is not active");
- players[playerIndex] = address(0);
+ activityStatus[playerAddress] = false;
+ payable(msg.sender).sendValue(entranceFee);
​
emit RaffleRefunded(playerAddress);
}

When drawing the winner, the players activity status can be checked, and only allow active players to be drawn as the winner.

Tools Used

Updates

Lead Judging Commences

Hamiltonite Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

denial-of-service-in-enter-raffle

Support

FAQs

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