Last Man Standing

Violation of Checks-Effects-Interactions Pattern in withdrawWinnings Creates Re-entrancy Risk

Description

• Normal Behavior: A winner withdrawing their prize should have their pendingWinnings balance set to zero first, and then the ETH should be transferred to them. This is known as the Checks-Effects-Interactions (CEI) pattern.

• The Issue: The withdrawWinnings function performs the external call to send ETH (payable(msg.sender).call{value: amount}("")) before it updates the user's internal balance (pendingWinnings[msg.sender] = 0;). While the function is protected by a nonReentrant modifier, this ordering is a severe violation of security best practices. If the nonReentrant guard were ever removed, flawed, or bypassed via a cross-function re-entrancy attack, a malicious contract could repeatedly call withdrawWinnings to drain the contract of all available funds before its balance is set to zero.

Risk

Likelihood: LOW

• Low (with the current nonReentrant guard in place). Medium-High (if the guard is ever removed or a more complex contract interaction allows a bypass). It represents a significant latent vulnerability.

Impact: HIGH

• A successful re-entrancy attack would allow a malicious winner to drain the contract's entire balance of winnings, stealing funds intended for other past or future winners.

Proof of Concept

A malicious actor could create a contract that does the following:

1. Plays and wins the game, having a balance in pendingWinnings.

2. Calls withdrawWinnings() from the malicious contract.

3. The Game contract sends ETH, triggering the malicious contract's receive() function.

4. Inside the receive() function, the malicious contract would try to call withdrawWinnings() again.

5. Observation: The current nonReentrant guard will stop this simple attack. However, the dangerous pattern remains. The security of the withdrawal function relies solely on the perfection of the re-entrancy guard, not on a fundamentally secure operational sequence.

Recommended Mitigation

Strictly adhere to the Checks-Effects-Interactions (CEI) pattern. Update the internal state (Effect) before the external call (Interaction).

Refactor withdrawWinnings() as follows: