getActivePlayerIndex() returns 0 for both the player at index 0 and a non-player, so callers cannot tell them apart
Root + Impact
Description
PuppyRaffle.getActivePlayerIndex() in src/PuppyRaffle.sol returns 0 in two completely different situations: when the player is genuinely at index 0 in the players array, and when the address is not a player at all. Callers have no way to tell those two cases apart from the return value alone.
The vulnerable code is:
The function loops through the array and returns the index when it finds the player, which is correct. But if the loop finishes without finding the player, it returns 0 as a fallback, and 0 is also a valid index: it is the index of the very first player to enter the raffle. The function therefore uses the same return value for "found at position 0" and "not found at all", which is an ambiguous sentinel value. There is no second return value, no boolean flag, and no revert to signal that the player was not found, so a caller that trusts the return value will treat a non-player exactly as if they were the first player.
Risk
Likelihood: High - the ambiguity is deterministic and happens on every call for a non-player. Anyone who is not in the raffle, and anyone checking an address that has already refunded, will receive 0.
Impact: Low - the direct damage is limited, because refund() checks that players[playerIndex] is the caller, so a non-player who tries to refund index 0 will usually revert unless they happen to be the first player. The real harm is to correctness and usability: front-ends, scripts, and other contracts that rely on this view function can display the wrong status, report that a non-player is active, or attempt a refund that will fail and waste gas. A user who has already refunded can be shown as still being at index 0, and a user who never entered can be shown as a player. That kind of misleading view function erodes trust in the raffle and can cause failed transactions and confused users, even if it does not directly move funds.
Proof of Concept
Manual trace - no tests were run; this is a step-by-step reading of the code, with a worked example.
Worked example: suppose Alice is the first player to enter, so players[0] is Alice, and Bob has never entered the raffle.
A front-end calls
getActivePlayerIndex(Alice). The loop finds Alice ati = 0and returns0.The same front-end calls
getActivePlayerIndex(Bob). The loop checks every entry, finds no match, falls through, and returns0as well.The front-end now has the same answer,
0, for both calls, and cannot tell that Alice is an active player while Bob is not a player at all. If it treats a return value of0as "this address is an active player", it will wrongly show Bob as active.If Bob then tries to use that index and calls
refund(0), the call reverts, becauseplayers[0]is Alice and not Bob, so Bob pays gas for a transaction that was always going to fail. The same confusion applies to an address that entered and then refunded: their slot isaddress(0), so the loop finds no match, and the function again returns0, wrongly suggesting they are still the first player.
Recommended Mitigation
Do not use a valid index as the "not found" value. Return a value that cannot be a real index, revert when the player is not found, or return a boolean alongside the index so callers can check.
Alternatively, return an int256 and use -1 for "not found", or revert with a clear error such as "PuppyRaffle: Player is not active". The contract already has a separate _isActivePlayer helper, so callers should also be encouraged to check activity with that function before trusting an index.
Lead Judging Commences
[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"); ```
Rewards breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Judging
Submissions are being reviewed by our AI judge. Results will be available in a few minutes.
View all submissionsRewards Distribution
The contest is complete and the rewards are being distributed.