Root + Impact
Description
Normal behavior: Players should be able to claim the throne by paying the required fee. The first player to claim becomes the king, and subsequent players can dethrone the current king by paying an increased fee.
Issue: The claimThrone() function contains a backwards logic check that prevents any player from claiming the throne, making the entire game unplayable.
@>
function claimThrone() external payable gameNotEnded nonReentrant {
require(msg.value >= claimFee, "Game: Insufficient ETH sent to claim the throne.");
require(msg.sender == currentKing, "Game: You are already the king. No need to re-claim.");
Risk
Likelihood: High
-
This bug triggers on EVERY attempt to claim the throne
-
The game starts with 'current = address(0)'
-
No real address equals 'address(0)', so all claims will revert
-
The game cannot start and will remain stuck forever
Impact: High
-
The entire game contract is non-functional
-
No player can participate in the game
-
Any ETH sent to the contract through other means could be locked forever
-
The contract deployment gas fees are wasted
-
Complete loss of protocol functionality
Proof of Concept
This test demonstrates that no player can claim the throne due to the backwards logic check.
The test attempts to claim the throne with three different player addresses, and all attempts
fail with the same error message. The game remains stuck with `currentKing = address(0)` forever,
proving the contract is completely non-functional.
*/
function testGameIsCompletelyBroken() public {
assertEq(game.currentKing(), address(0));
address[] memory players = new address[](3);
players[0] = player1;
players[1] = player2;
players[2] = player3;
for(uint i = 0; i < players.length; i++) {
vm.startPrank(players[i]);
vm.expectRevert("Game: You are already the king. No need to re-claim.");
game.claimThrone{value: INITIAL_CLAIM_FEE}();
vm.stopPrank();
}
assertEq(game.currentKing(), address(0));
}
Recommended Mitigation
function claimThrone() external payable gameNotEnded nonReentrant {
require(msg.value >= claimFee, "Game: Insufficient ETH sent to claim the throne.");
- require(msg.sender == currentKing, "Game: You are already the king. No need to re-claim.");
+ require(msg.sender != currentKing, "Game: You are already the king. No need to re-claim.");
/*
The require statement currently checks if the caller IS the current king (==) and reverts if true.
This prevents anyone except the current king from claiming the throne. Since the game starts with
currentKing = address(0), and no real address can equal address(0), no one can ever claim the throne.
By changing the operator to != (not equal), the logic correctly prevents the current king from reclaiming
their own throne while allowing other players to claim it, which is the intended game behavior.
*/
