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

Ambiguous return value in getActivePlayerIndex causes logic errors

alaaeldinsabib

Summary

The getActivePlayerIndex function in PuppyRaffle.sol returns 0 both when a player is found at index 0 AND when a player is not found in the raffle. This ambiguity makes it impossible for external callers (like a frontend or external contracts) to distinguish between a valid player at the first position and a non-existent player, leading to potential logic errors and failed refunds.

Description

The function uses 0 as a sentinel value to indicate "player not found". However, 0 is also a valid array index. When the first player to enter the raffle (at index 0) tries to check their status or when an external system tries to verify their participation, the function returns 0. The caller cannot know if this means "You are player #0" or "You are not in the raffle".

This is particularly dangerous for the refund function, which relies on knowing the correct index. If a user at index 0 is mistakenly identified as "not found", they might be blocked from refunding, or conversely, if a non-player gets index 0, they might attempt invalid operations.

Root Cause

File: src/PuppyRaffle.sol (lines 113-120)

function getActivePlayerIndex(address player) external view returns (uint256) {
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player) {
return i; // ✅ Returns 0 if player is at index 0
}
}
return 0; // ❌ Also returns 0 if player is NOT found
}

Risk

Severity: Medium
Likelihood: Medium
Impact: Low

  • ❌ Ambiguous return value causes confusion

  • ❌ External systems cannot reliably check player status

  • ❌ Potential for failed refunds or incorrect logic

  • ✅ Does not directly lose funds, but degrades UX and reliability

Proof of Concept

Scenario: Player A enters the raffle first (index 0). Player B (who never entered) checks their index. Both get 0 as the result.

function test_GetActivePlayerIndexAmbiguity() public {
address playerZero = address(1);
address nonExistentPlayer = address(999);
// Step 1: Player A enters the raffle (will be at index 0)
address[] memory players = new address[](1);
players[0] = playerZero;
raffle.enterRaffle{value: entranceFee}(players);
// Step 2: Check index for the actual player at index 0
uint256 indexForPlayerZero = raffle.getActivePlayerIndex(playerZero);
// Step 3: Check index for a player who never entered
uint256 indexForNonExistent = raffle.getActivePlayerIndex(nonExistentPlayer);
// Step 4: Both return 0!
assertEq(indexForPlayerZero, 0);
assertEq(indexForNonExistent, 0);
console.log("Player at index 0 returns:", indexForPlayerZero);
console.log("Non-existent player returns:", indexForNonExistent);
console.log("VULNERABILITY: Ambiguous return value makes them indistinguishable!");
}

Test Output:

Player at index 0 returns: 0
Non-existent player returns: 0
VULNERABILITY: Ambiguous return value makes them indistinguishable!

What This Proves:

  1. ✅ Function returns 0 for both valid index 0 and "not found"

  2. ✅ Callers cannot distinguish between the two cases

  3. ✅ Leads to unreliable external integrations

Recommended Mitigation

Change the return type to include a boolean flag, or use a sentinel value like type(uint256).max for "not found".

// Fix 1: Return a tuple with a boolean flag (Recommended)
function getActivePlayerIndex(address player) external view returns (uint256, bool) {
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player) {
return (i, true); // ✅ Found at index i
}
}
return (0, false); // ✅ Not found
}
​
// Fix 2: Use type(uint256).max as sentinel value
function getActivePlayerIndex(address player) external view returns (uint256) {
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player) {
return i;
}
}
return type(uint256).max; // ✅ Clear "not found" value
}

Why This Fixes It:

  1. ✅ Eliminates ambiguity completely

  2. ✅ External systems can reliably check player status

  3. ✅ Prevents logic errors in refund and other flows

References

  • CWE-394: Unexpected Status Code or Return Value

  • Solidity Best Practices for Error Handling

Updates

Lead Judging Commences

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

[L-01] Ambiguous index returned from PuppyRaffle::getActivePlayerIndex(address), leading to possible refund failures

## Description The `PuppyRaffle::getActivePlayerIndex(address)` returns `0` when the index of this player's address is not found, which is the same as if the player would have been found in the first element in the array. This can trick calling logic to think the address was found and then attempt to execute a `PuppyRaffle::refund(uint256)`. ## Vulnerability Details The `PuppyRaffle::refund()` function requires the index of the player's address to preform the requested refund. ```solidity /// @param playerIndex the index of the player to refund. You can find it externally by calling `getActivePlayerIndex` function refund(uint256 playerIndex) public; ``` In order to have this index, `PuppyRaffle::getActivePlayerIndex(address)` must be used to learn the correct value. ```solidity /// @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, if they are not active, it returns 0 function getActivePlayerIndex(address player) external view returns (int256) { // find the index... // if not found, then... return 0; } ``` The logic in this function returns `0` as the default, which is as stated in the `@return` NatSpec. However, this can create an issue when the calling logic checks the value and naturally assumes `0` is a valid index that points to the first element in the array. When the players array has at two or more players, calling `PuppyRaffle::refund()` with the incorrect index will result in a normal revert with the message "PuppyRaffle: Only the player can refund", which is fine and obviously expected. On the other hand, in the event a user attempts to perform a `PuppyRaffle::refund()` before a player has been added the EvmError will likely cause an outrageously large gas fee to be charged to the user. This test case can demonstrate the issue: ```solidity function testRefundWhenIndexIsOutOfBounds() public { int256 playerIndex = puppyRaffle.getActivePlayerIndex(playerOne); vm.prank(playerOne); puppyRaffle.refund(uint256(playerIndex)); } ``` The results of running this one test show about 9 ETH in gas: ```text Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest [FAIL. Reason: EvmError: Revert] testRefundWhenIndexIsOutOfBounds() (gas: 9079256848778899449) Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 914.01µs ``` Additionally, in the very unlikely event that the first player to have entered attempts to preform a `PuppyRaffle::refund()` for another user who has not already entered the raffle, they will unwittingly refund their own entry. A scenario whereby this might happen would be if `playerOne` entered the raffle for themselves and 10 friends. Thinking that `nonPlayerEleven` had been included in the original list and has subsequently requested a `PuppyRaffle::refund()`. Accommodating the request, `playerOne` gets the index for `nonPlayerEleven`. Since the address does not exist as a player, `0` is returned to `playerOne` who then calls `PuppyRaffle::refund()`, thereby refunding their own entry. ## Impact 1. Exorbitantly high gas fees charged to user who might inadvertently request a refund before players have entered the raffle. 2. Inadvertent refunds given based in incorrect `playerIndex`. ## Recommendations 1. Ideally, the whole process can be simplified. Since only the `msg.sender` can request a refund for themselves, there is no reason why `PuppyRaffle::refund()` cannot do the entire process in one call. Consider refactoring and implementing the `PuppyRaffle::refund()` function in this manner: ```solidity /// @dev This function will allow there to be blank spots in the array function refund() public { require(_isActivePlayer(), "PuppyRaffle: Player is not active"); address playerAddress = msg.sender; payable(msg.sender).sendValue(entranceFee); for (uint256 playerIndex = 0; playerIndex < players.length; ++playerIndex) { if (players[playerIndex] == playerAddress) { players[playerIndex] = address(0); } } delete existingAddress[playerAddress]; emit RaffleRefunded(playerAddress); } ``` Which happens to take advantage of the existing and currently unused `PuppyRaffle::_isActivePlayer()` and eliminates the need for the index altogether. 2. Alternatively, if the existing process is necessary for the business case, then consider refactoring the `PuppyRaffle::getActivePlayerIndex(address)` function to return something other than a `uint` that could be mistaken for a valid array index. ```diff + int256 public constant INDEX_NOT_FOUND = -1; + function getActivePlayerIndex(address player) external view returns (int256) { - function getActivePlayerIndex(address player) external view returns (uint256) { for (uint256 i = 0; i < players.length; i++) { if (players[i] == player) { return int256(i); } } - return 0; + return INDEX_NOT_FOUND; } function refund(uint256 playerIndex) public { + require(playerIndex < players.length, "PuppyRaffle: No player for index"); ```

Support

FAQs

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

Give us feedback!