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

`puppyRaffle::getActivePlayerIndex` returns index `0` if the player is not active

ciara

PuppyRaffle::getActivePlayerIndex checks if a player is active by looping through the players array and returning the index at which the address provided matches the address at the current index. If the address is not found in the array, the function returns 0. Since Solidity arrays are 0 indexed, the first player in the players array will also return an index of 0. This makes it seem as though the non-active player is the first player even though they have not joined the raffle.

Vulnerability Details

In PuppyRaffle::getActivePlayerIndex, on line 110-117, the address provided as an argument is checked against the players array to check if they match if they do, the index is returned:

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

If the player address provided matches the first address in the players array, an index of 0 will be returned. However, if no address is found to match in the players array, 0 is also returned, erroneously making it seem as though the provided address is the first address in the players array.

Impact

If a user is attempting to determine their index in the raffle, they may think they have entered as the first player even though they have yet to enter the raffle. This function could also be used externally to retrieve the player's index before using it to call refund() leading to revert as the caller is not the first element in the array, wasting the user's gas.

This function is not used internally so does not cause any downhill errors however incorrect, externally facing APIs should be fixed to avoid confusion and wasting gas, which makes this finding a low-severity vulnerability.

Proof of Concept

Working Test Case

The following test enters players in a raffle and then checks for the index of an address that has not entered the raffle. The test passing demonstrates that an index of 0 has been returned even though the address provided has not entered the raffle:

function test_poc_playerIndexZeroReturnedForNonPlayer() public playersEntered {
address inactivePlayer = makeAddr("inactivePlayer");
uint256 index = puppyRaffle.getActivePlayerIndex(inactivePlayer);
assertEq(index, 0);
}

Running the test yields the following output:

$ forge test --mt test_poc_playerIndexZeroReturnedForNonPlayer
​
// output
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] test_poc_playerIndexZeroReturnedForNonPlayer() (gas: 152236)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.77ms

The test passes showing that an index of 0 has incorrectly been returned.

Recommended Mitigation

Either remove the necessity for users to determine their index in the array by changing the argument for refund() to be an address and checking the address' activity status before proceeding with the refund or if no index is found to match, revert with the following statement:

function getActivePlayerIndex(address player) external view returns (uint256) {
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player) {
return i;
}
}
- return 0;
+ revert("Player has not yet entered the lottery");
}

Tools Used

Updates

Lead Judging Commences

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

getActivePlayerIndex can say a player is both entered at slot 0 and inactive

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

getActivePlayerIndex can say a player is both entered at slot 0 and inactive

Support

FAQs

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