Last Man Standing
First Flight #45
Beginner FriendlyFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: medium
Invalid

Potential reentrancy in function Game::withdrawWinnings

mozi

Potential reentrancy in function Game::withdrawWinnings

Description

If winner player is an a smart contract the player can reenter using receive function in their smart contract, but since most of the mutable function on the Game contract it's have modifier that prevent reentrant to those function(i.e nonReentrant or onlyOwner). It is also can be more dangerous in the future, when Game contract may inherated more dependencies that can posibilly modify any state in the Game contracts.

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender];
require(amount > 0, "Game: No winnings to withdraw.");
​
(bool success, ) = payable(msg.sender).call{value: amount}(""); // external call
require(success, "Game: Failed to withdraw winnings.");
​
//@audit-issue reentrancy
@> pendingWinnings[msg.sender] = 0; // state updated after an external call
​
emit WinningsWithdrawn(msg.sender, amount);
}

Risk

Likelihood:

  • when winner player is an a smart contract and have receive function that posibilly call malicious function or any action that can posibily drain funds or modify some state variable

Impact:

  • No loss of funds or modify state variable when attacker reenter for now

  • future risk, when Game contract may inherated more dependencies that can posibilly modify any state in the Game contracts and attacker can call any function from dependencies that can modify state or steal/drains funds Game contract balance.

Recommended Mitigation

Follow CEI(Checks Effects Interactions) pattern to prevent reentrancy:

function withdrawWinnings() external nonReentrant {
// Checks
uint256 amount = pendingWinnings[msg.sender];
require(amount > 0, "Game: No winnings to withdraw.");
​
// Effects
+ pendingWinnings[msg.sender] = 0;
​
- (bool success, ) = payable(msg.sender).call{value: amount}(""); // external call
- require(success, "Game: Failed to withdraw winnings.");
​
- pendingWinnings[msg.sender] = 0; // state updated after an external call
​
// Interactions
+ (bool success, ) = payable(msg.sender).call{value: amount}("");
+ require(success, "Game: Failed to withdraw winnings.");
​
emit WinningsWithdrawn(msg.sender, amount);
}
Updates

Appeal created

inallhonesty Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Game::withdrawWinnings reentrancy

We only audit the current code in scope. We cannot make speculation with respect to how this codebase will evolve in the future. For now there is a nonReentrant modifier which mitigates any reentrancy. CEI is a good practice, but it's not mandatory. Informational

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.