HardcoreGamble Audit

HardcoreGambleAudit.sol

// SPDX-License-Identifier: MIT // OK
pragma solidity ^0.8.12; // OK
import "./SafeMath.sol"; // OK
//rules are described in README.txt // OK
contract HardcoreGamble { // @audit HIGH SEVERITY: Tokenomics: An advanced party can always win after someone deposits by depositing with several larger stakes after them and calling withdraw with the largest of their stakes. The win requirements are not safe against collusion between multiple accounts. Using a flashloan this can be done exceptionally cheaply.
    address private owner; // @audit Bad practice: private variable that is important
    address private top_stake; //the participant with currently highest bid // @audit Bad practice: private variable that is important
    address private winner; // OK
    uint256 pool; // OK
    uint256 risk_pool; //the pool at the moment of the highest stake transaction // @audit Bad practice: private variable that is important
    mapping(address => uint256) public balances;
    mapping(address => bool) entered; // @audit Bad practice: private variable that is important
    bool public highball; //situation when someones bid is higher then half of a pool
    bool public lowball; //situation when someones bid is less then half of a pool
    bool finished; // @audit Bad practice: private variable that is important

    constructor() { // OK
        owner = msg.sender; // @note The deployer of the contract is the initial owner 
        pool = 0; // @audit Bad practice: Initializing variables to zero makes no sense
        finished = false; // @audit Bad practice: Initializing variables to zero makes no sense
        balances[top_stake] = 0; // @audit Bad practice: Initializing variables to zero makes no sense
        highball = false; // @audit Bad practice: Initializing variables to zero makes no sense
        lowball = false; // @audit Bad practice: Initializing variables to zero makes no sense
        risk_pool = pool; // @audit Bad practice: Initializing variables to zero makes no sense
    }

    modifier restricted() { // OK
        if (msg.sender == owner) _; // @audit Bad practice: Does not revert. use `require(msg.sender == owner, "only owner");`
    } // OK
    uint256 number_of_participants; // OK
    address[] participants; // OK

    function enterPool() public payable { // @audit `enterPool` can be made external // @note Users can call `enterPool` with an ETHEREUM amount to participate. `pool` keeps track of the total stake and balances[user] keeps track of the user balance.
        require(finished == false, "The game has ended..."); // @audit Style: !finished is arguably shorter here.
        //require(  
        //    msg.value >= 0.1 ether,
        //    "You have to put at least one Ether..."
        //);
        require(entered[msg.sender] == false, "You have already entered...");// @audit Style: !entered[msg.sender] is arguably shorter here.
        participants.push(msg.sender); // @note participants is correctly validated against double entries (#L39)
        entered[msg.sender] = true; // OK
        balances[msg.sender] += msg.value; // @note all though in my opinion good practice to increment this, assignment would have sufficed due to the entered lock.
        pool += msg.value; // OK

        number_of_participants = SafeMath.add(1, number_of_participants); // @audit Bad practice: Usage of safemath on 0.8.12 makes little sense as 0.8.12 is safe against overflow in this scenario.
        if(number_of_participants>=3){ // OK
            chooseWinner(); // @note After a total of three participants have entered, the winner is chosen. 
        } // OK
    }

    function chooseWinner() private { // @note Should the game not be finished at the point of chooseWinner? People can still enter after a winner has been chosen. This is likely intentional given the comments and greater than or equal sign.
        require( // OK
            number_of_participants >= 3, // @audit Bad practice: requirements which cannot fail should use `assert`.
            "You have to wait for more participants (at least 3)" // OK
        ); // OK
        uint256 dif;
        address risk_top_stake; //holds the top bid participant to check if changed 
        //*the participant with the risk of winning
        for (uint256 i = 0; i < number_of_participants; i++) { // @audit MEDIUM SEVERITY: DoS: A malicious party can call `enterPool` 1000s of times to cause this for loop to consume more gas than a single block is allowed to use. For loops are generally bad practice.
            if (balances[participants[i]] > balances[top_stake]) { // @note The winning requirement is that the user with the highest stake will win.
                top_stake = participants[i]; // @note `top_stake` resembles the user with the highest stake. In case of a tie the first staker wins.
                if (top_stake != risk_top_stake) { // @audit This checks seems useless? When will top_stake ever equal risk_top_stake ?
                    if (balances[top_stake] < SafeMath.sub(pool,balances[top_stake])) { // @note if topstake > total - topstake: If the top stake owns more than the rest of the whole pool
                        lowball = true; // @note In case the top stake owns more than the rest of the whole pool, lowball is set to true and highball to false.
                        highball = false;
                        dif = pool-balances[top_stake]; // holds the difference between pool and highest stake 
                    } else {
                        highball = true;  // @note In case the top stake owns less than the rest of the whole pool, highball is set to true and lowball to false.
                        lowball = false; // @audit dif is not set here, is this a problem? Probably not given that the comment above says it should always be pool - highest stake.
                    }
                    risk_top_stake = top_stake; // @note `risk_top_stake` is set to the previous `top_stake`
                    risk_pool = pool; // @note Sum of all user balances before the top staker.
                }

            }
        }
        if (lowball == true) { // @note if the user owns more than the remainder of the pool, they are only eligible to win in case:  pool > (risk_pool- top_stake)*2 (the remainder of the stakers before the user times two must be smaller than the pool, eg. enough people have come after them) 
            if (pool>SafeMath.mul((risk_pool-balances[top_stake]),2 )) {winner = top_stake;} // @audit Tokenomics: There's no incentive to enter with a smaller stake than the top_stake. Arguably this case should not occur in a rational practice.
        }
        if (highball == true) { // @audit it might be sufficient to put this as an else or as an else if. Within a professional codebase a WinnerType enum might be used instead as two booleans is not clean code.
            if (SafeMath.sub(pool, balances[top_stake]) > balances[top_stake]) {winner = top_stake;} // @audit kind of fringe scenario, this only really prevents the top-stake from winning here in case the top stake owns exactly what the rest of the pool owns. // @note if the top user does does not own most of the pool: pool - top_stake > top_stake, then the remainder must be larger than the top_stake.
        }
    }
//ONLY FOR THE SAKE OF TESTING//
    function getNumberOfParticipants()public view restricted  // @audit `getWinner` can be marked as external.
        returns (uint256 a)  // @audit Style: Just return (uint256) instead of (uint256 a)
    {
        return number_of_participants; // OK
    }
    function getWinner() public view restricted returns (address a) { // @audit `getWinner` can be marked as external.  // @audit Style: Just return (uint256) instead of (uint256 a)
        return winner; // OK
    }

    function getPool() public view restricted returns (uint256 a) { // @audit `getPool` can be marked as external. // @audit Style: Just return (uint256) instead of (uint256 a)
        require(msg.sender == owner); // @audit Bad practice: Adding privileges to view functions makes no sense.
        return pool; // OK
    }
//////////////////////////////////
    function withdraw() public payable { // @audit `withdraw` can be marked as external.
        require(msg.sender == winner, "You are not the winner");  // @note `address(0)` can withdraw early, not a big deal as it is unlikely owned.
        require(finished == false, "You took the prize, game ended."); // @audit Style: `!finished` is arguably shorter here.
        payable(msg.sender).transfer(pool); // @audit HIGH SEVERITY: `withdraw` is vulnerable to reentrancy (arguable it cannot be exploited as `.transfer` most likely does not provide enough gas to abuse it).
        finished = true; // OK
    }
}