Puppy Raffle

## 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"); ```