Root + Impact
Description
-
Normal behavior:
Winners should always be able to withdraw their winnings regardless of whether they are EOAs or smart contracts.
Specific issue:
The withdrawWinnings()
function uses a low-level call to transfer ETH. If the recipient is a contract that reverts on ETH reception, the withdrawal fails permanently, locking the funds forever.
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}("");
require(success, "Game: Failed to withdraw winnings.");
pendingWinnings[msg.sender] = 0;
emit WinningsWithdrawn(msg.sender, amount);
}
Risk
Likelihood:
Impact:
Winner's funds are permanently locked with no recovery mechanism.
No way to force transfer the funds to the rightful winner.
Breaks the core game promise that winners can claim their prize.
Proof of Concept
contract RevertingContract {
function claimThrone(Game game) external payable {
game.claimThrone{value: msg.value}();
}
}
function testWithdrawalDoSRevertingContract() public {
revertingContract.claimThrone{value: INITIAL_CLAIM_FEE}(game);
vm.warp(block.timestamp + GRACE_PERIOD + 1);
game.declareWinner();
assertTrue(game.pendingWinnings(address(revertingContract)) > 0, "Should have winnings");
vm.startPrank(address(revertingContract));
vm.expectRevert("Game: Failed to withdraw winnings.");
game.withdrawWinnings();
vm.stopPrank();
assertTrue(game.pendingWinnings(address(revertingContract)) > 0, "Winnings still locked");
}
Recommended Mitigation
Implement a pull payment pattern with fallback mechanism or allow owner to recover stuck funds:
+ mapping(address => bool) public withdrawalFailed;
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}("");
- require(success, "Game: Failed to withdraw winnings.");
- pendingWinnings[msg.sender] = 0;
+
+ if (success) {
+ pendingWinnings[msg.sender] = 0;
+ emit WinningsWithdrawn(msg.sender, amount);
+ } else {
+ withdrawalFailed[msg.sender] = true;
+ emit WithdrawalFailed(msg.sender, amount);
+ }
}
+ // Allow owner to recover funds for failed withdrawals
+ function recoverFailedWithdrawal(address winner, address newRecipient) external onlyOwner {
+ require(withdrawalFailed[winner], "No failed withdrawal");
+ uint256 amount = pendingWinnings[winner];
+ pendingWinnings[winner] = 0;
+ withdrawalFailed[winner] = false;
+ payable(newRecipient).transfer(amount);
+ }