VulnerableC4LotteryAns

VulnerableC4LotteryAns.sol

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.13;

/// @dev interfaces have been minimalised to contain only relevant functions
interface IERC20 {
    function transferFrom(address from, address to, uint value) external returns (bool);
}

interface IWeth is IERC20 {
    function withdraw(uint) external;
}

/// Lottery contract that requires users to guess a randomly generated number from 0 to 99_999
/// A user can make multiple guesses, with each guess costing 1 eth / weth
/// A user can also withdraw their guess and be refunded half of GUESS_COST
/// First person to call winLottery() gets entire balance
contract C4Lottery {
    IWeth weth;
    // each guess costs 1 ETH
    uint256 constant GUESS_COST = 1 ether;
    // list of lottery participants
    address[] public participants;
    // end block number for guessing period
    uint32 public endBlockNum;
    // guesses of each participiant
    mapping(address => uint16[]) public guesses;
    // total deposited funds per guesser
    mapping(address => uint256) public funds;

    /// @param _weth canonical wrapped native asset contract
    /// @param _guessDuration duration for which guess can be submitted, denominated in 100 block chunks
    constructor(IWeth _weth, uint8 _guessDuration) {
        require(_guessDuration > 0, '0 guess duration');
        weth = _weth;
        /// @audit: calculation here will likely overflow because 100 literal fits into type uint8
        /// since _guessDuration is also of type uint8, an uint8 is allocated to hold the result
        /// thus max value for multiplication is of type uint8 = 255
        /// note: special thanks to pauliax | thunder for this bug recommendation =)
        endBlockNum = uint32(block.number) + _guessDuration * 100;
    }
    
    modifier beforeEnd() {
        require(uint32(block.number) < endBlockNum, 'guessing period ended');
        _;
    }

    // @audit: Missing receive() function as ETH is expected from WETH contract (only)

    // make a guess with ETH
    /// @param entry guess number 
    function guessEth(uint16 entry) public payable beforeEnd {
        // @audit: change to == instead, since user will only be credited GUESS_COST
        require(msg.value >= GUESS_COST, 'insufficient funds sent');
        _guess(entry);
    }
    
    // make a guess with WETH
    /// @param entry guess number
    function guessWeth(uint16 entry) public payable beforeEnd {
        weth.transferFrom(msg.sender, address(this), GUESS_COST);
        // unwrap WETH
        weth.withdraw(GUESS_COST);
        _guess(entry);
    }

    function makeMultipleGuesses(uint16[] calldata entries, bool isEth) public payable beforeEnd {
        uint256 i;
        uint256 entriesLength = entries.length;
        for (i; i < entriesLength; ++i) {
            // @audit using msg.value in a loop means multiple guesses can be made with the cost of 1 guess
            // @audit (gas op): either make isEth boolean array or select function outside the loop
            // bytes4 func = isEth ? this.guessEth.selector : this.guessWeth.selector
            // address(this).call(abi.encodeWithSelector(func, entries[i]));
            isEth ? guessEth(entries[i]) : guessWeth(entries[i]);
        }
    }
    
    /// @param index guess index in user's guesses array
    // @audit: missing beforeEnd modifier
    // users shouldnt be allowed to withdraw after endBlockNum
    // because the winning number can be determined by then
    function withdrawGuess(uint256 index) public payable {
        // @audit: this check can be replaced with a better check
        // that the user's parsed index is within the guess length
        require(funds[msg.sender] >= GUESS_COST, 'user did not participate');
        // remove guess
        // @audit: this merely zeroes the value, but isn't removed from the array
        // can call this function multiple times with the same index
        delete guesses[msg.sender][index];
        // process refund
        // @audit use .call() instead of .transfer() because it forwards all gas to recipient
        // as opposed to .transfer() of only 2300 gas
        // @audit: contract description says half of GUESS_COST, not full amount
        payable(msg.sender).transfer(GUESS_COST);
        // @audit: I wanted this to be vulnerable to reentrancy
        // but wardens @0xDjango and @hyh (and @cmichel) pointed out
        // that this is a false positive
        funds[msg.sender] -= GUESS_COST;

        // remove user from participant list if no more guesses remaining
        if (guesses[msg.sender].length == 0) {
            uint256 i;
            uint256 numParticipants = participants.length - 1;
            // @audit: might run out of gas if participant list is too long
            // in these cases it might make sense to take index as a fn parameter
            // @audit: Warden @Dravee pointed out that this will revert for base case i == 0
            // but he was unfortunately too shy to mention it in person =p
            for (i = numParticipants; i >= 0; i--) {
                // @audit: missing bracket means partcipants.pop() gets executed each iteration
                if (participants[i] == msg.sender)
                    participants[i] = participants[numParticipants];
                    participants.pop();
                    // @audit: missing a break (in this case, return works too) statement
            }
        }
    }
    
    // whoever made a correct guess and is first to call this function after the guess period
    // will be able to withdraw entire contract balance
    function winLottery(uint256 index) external {
        require(uint32(block.number) >= endBlockNum, 'guessing period has not ended');
        // @audit using blockhash is bad for RNG
        // only the last 256 block hashes are available
        // meaning calling this function after 256 blocks are mined would be
        // uint256(keccak256(0x0)) % 100_000 = 2947
        // @audit guesses are uint16, max number possible is 65_535
        require(guesses[msg.sender][index] == uint256(keccak256(abi.encodePacked(blockhash(endBlockNum)))) % 100_000);
        payable(msg.sender).transfer(address(this).balance);
    }

    function getGuesses(address user) public view returns(uint16[] memory) {
        return guesses[user];
    }
    
    function _guess(uint16 number) internal {
        // if new guesser, add to participant list
        if (guesses[msg.sender].length == 0) {
            participants.push(msg.sender);
        }
        funds[msg.sender] += GUESS_COST;
        guesses[msg.sender].push(number);
    } 
}