withdrawWinnings() Does Not Follow the Checks-Effects-Interactions (CEI) Pattern, Leading to Potential Re-Entrancy Risk

Description

• In WithdrawWinning() ,the line that resets pendingWinnings[msg.sender] to 0 is placed after the external call.

• This violates the Checks-Effects-Interactions (CEI) pattern and introduces a potential re-entrancy vulnerability. If a malicious contract receives ETH via the low-level call, it could re-enter the withdrawWinnings() function before its pendingWinnings is cleared.

• As a result, the attacker can bypass the amount > 0 check and repeatedly drain the contract’s balance.

Risk

Likelihood:High

• The function was calling the external call to another address,when the recipient is a contract, the fallback or receive function is triggered, allowing it to perform arbitrary logic, including re-entrant calls.

Impact:High

• Since the require(amount > 0) is useless , the attacker can repeatedly call the withdrawWinnings() till the balance of the Game contract is drained. so the attacker is successfully to steal all the ether which does not belong to him.

• Note: While nonReentrant provides protection against direct re-entrancy in the current context, relying solely on modifiers is not always sufficient.

  For example, if future changes introduce internal calls, delegatecalls, or access through another entry point, the low-level call may still become a re-entrancy vector.

  In addition, the fallback or receive function of the recipient may contain side effects that interfere with contract logic or gas usage, even without re-entrancy.

Proof of Concept

A malicious contract can exploit the low-level call as follows:

In this example, when the Game contract sends ETH to the malicious contract, it triggers the fallback function, which re-enters the vulnerable withdrawWinnings() logic before the previous withdrawal finalizes.And while the pendingWinnings[msg.sender] was not be set to zero, so it can still bypass the amount > 0 check until the contract’s entire balance is drained.

Recommended Mitigation

• Following the Checks-Effects-Interactions (CEI) pattern will protect contracts from re-entrancy issues.

• Placed the line that resets the variable pendingWinner before the external call.