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

[L-1] Ambiguous Return Value in getActivePlayerIndex Causes Incorrect Refunds

0xaljzoli

[L-1] Ambiguous Return Value in getActivePlayerIndex Causes Incorrect Refunds

Title: Ambiguous Return Value in getActivePlayerIndex Causes Incorrect Refunds

Impact: Low

Likelihood: Medium

Description

  • The getActivePlayerIndex function is intended to help players find their position in the players array so they can call refund with the correct index. It should return the array index of a given player address if they are active, or indicate that they are not in the raffle.

  • The function returns 0 both when a player is found at index 0 and when a player is not found at all. This creates an ambiguity: a player legitimately at index 0 and a player not in the raffle receive the same return value. If a player not in the raffle calls refund(0), they will actually attempt to refund the player at index 0, which will revert due to the require(playerAddress == msg.sender) check. However, the real risk is reversed — a player at index 0 who does not verify their position may be unable to distinguish whether they are actually in the raffle or not.

function getActivePlayerIndex(address player) external view returns (uint256) {
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player) {
return i;
}
}
- return 0;
+ // @> Returns 0 for both "player found at index 0" AND "player not found"
+ // @> Callers cannot distinguish between the two cases
}

Risk

Likelihood:

  • Any player at index 0 who uses getActivePlayerIndex to verify their participation will receive 0, which is the same value returned for players not in the raffle. This is a design flaw that affects every raffle round since index 0 is always occupied.

  • Frontend applications or off-chain integrations relying on getActivePlayerIndex to check whether a player is active will produce incorrect results. An off-chain service checking getActivePlayerIndex(myAddress) == 0 cannot determine if the player is at index 0 or simply not in the raffle.

Impact:

  • The ambiguity does not directly lead to fund loss because refund has a require(playerAddress == msg.sender) guard. However, it causes poor user experience where a legitimate player at index 0 may believe they are not in the raffle and miss the opportunity to refund.

  • More critically, external integrations and frontend interfaces that use this function to display player status will show incorrect information, potentially leading to support issues, confusion, and loss of user trust in the platform.

Proof of Concept

function testAmbiguousPlayerIndex() public {
// PlayerOne enters at index 0
address[] memory players = new address[](1);
players[0] = playerOne;
puppyRaffle.enterRaffle{value: entranceFee}(players);
​
// PlayerOne is at index 0
uint256 indexOne = puppyRaffle.getActivePlayerIndex(playerOne);
assertEq(indexOne, 0); // Returns 0 — correct, but ambiguous
​
// PlayerTwo never entered the raffle
uint256 indexTwo = puppyRaffle.getActivePlayerIndex(playerTwo);
assertEq(indexTwo, 0); // Also returns 0 — indistinguishable!
​
// Both return 0 — caller cannot tell who is actually in the raffle
// A frontend checking (index == 0) would incorrectly treat
// PlayerTwo the same as PlayerOne
}

This PoC demonstrates that both a player legitimately at index 0 (playerOne) and a player not in the raffle at all (playerTwo) receive the identical return value of 0 from getActivePlayerIndex. Any off-chain code or user interface using this function to check membership or determine refund eligibility will produce incorrect results. While the on-chain refund function protects against direct exploitation due to its msg.sender check, the function's API contract is fundamentally broken and misleads integrators.

Recommended Mitigation

function getActivePlayerIndex(address player) external view returns (uint256) {
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player) {
return i;
}
}
- return 0;
+ return type(uint256).max; // Sentinel value indicating "not found"
}

Returning type(uint256).max as a sentinel value makes it impossible to confuse with a valid array index. No valid player array can have 2^256 - 1 entries, so this value is unambiguously "not found." Callers can now clearly distinguish between a player at index 0 and a player not in the raffle. Alternatively, the function could return a boolean alongside the index using a struct or a separate bool return value to make the API even more explicit and self-documenting.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 3 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!