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

`getActivePlayerIndex` doesn't behave as stated in NATSPEC

inallhonesty

Summary

getActivePlayerIndex doesn't behave as stated in NATSPEC because it doesn't treat the 0 edge case.

Vulnerability Details

The NATSPEC specifies the following @return the index of the player in the array, if they are not active, it returns 0.

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

But given that the players are tracked in a list, the index of the first player will always be 0. Thus when we call getActivePlayerIndex with the address of the first player the function will see that players[i] == player when i = 0, thus returns 0. But 0 is returned when a player is not active. This leads to confusion.

PoC

Copy this in PuppyRaffleTest.t.sol

function testGetActivePlayerIndexSinglePlayer() public {
address[] memory players = new address[](1);
players[0] = playerOne;
puppyRaffle.enterRaffle{value: entranceFee * 1}(players);
​
assertEq(puppyRaffle.getActivePlayerIndex(playerOne), 0);
}

Impact

Weird behavior of a function that doesn't behave like the NATSPEC indicates.

Tools Used

Manual review

Recommendations

Use this instead:

/// @notice a way to get the index in the array
/// @param player the address of a player in the raffle
/// @return the index of the player in the array, or -1 if they are not active
function getActivePlayerIndex(address player) external view returns (int256) {
for (int256 i = 0; i < int256(players.length); i++) {
if (players[uint256(i)] == player) {
return i;
}
}
return -1;
}

The edge case is treated because -1 is not a valid index in a Solidity array. No need to use SafeCast for int256(players.length) because it's impossible for that value to get too high.

Updates

Lead Judging Commences

Hamiltonite Lead Judge over 1 year 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.