First Flight #2: Puppy Raffle

Summary

LOW-7: Both functions perform the same operation but they return different values. This can be achieved using one function only, saving gas costs during deployment. It also would have no impact on security. _isActivePlayer function is marked as internal, but all it does is loop through a public array. It also is not used anywhere in the contract so there may even be a consideration to delete the function as such.

Vulnerability Details

Both functions perform the same operation but return different values. This can be achieved using one function only, making the code shorter, decreasing the deployment cost, and making the code more readable.

Impact

Lower gas cost, redability

Tools Used

Static analysis

Recommendations

R-1: Merge the functions to return a tuple of values (the downside is that when using the function in the code, it is necessary to use unpacking of the result). The final function may look similar to this:

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

R-2: Remove the _isActivePlayer function - it is not used in either the code or the tests