Incorrect Logic Results in Players Unable to Claim Throne
Description
-
In normal gameplay, any player other than the current king should be able to claim the throne by paying the current claimFee. If the caller is already the king, the function should revert to avoid unnecessary or redundant state updates.
-
However, the claimThrone() function incorrectly requires that the msg.sender must be the current king. This is the opposite of the intended behavior. As a result, only the current king can call claimThrone, which blocks all other players from participating and completely halts game progression.
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.");
uint256 sentAmount = msg.value;
uint256 previousKingPayout = 0;
uint256 currentPlatformFee = 0;
uint256 amountToPot = 0;
currentPlatformFee = (sentAmount * platformFeePercentage) / 100;
if (currentPlatformFee > (sentAmount - previousKingPayout)) {
currentPlatformFee = sentAmount - previousKingPayout;
}
platformFeesBalance = platformFeesBalance + currentPlatformFee;
amountToPot = sentAmount - currentPlatformFee;
pot = pot + amountToPot;
currentKing = msg.sender;
lastClaimTime = block.timestamp;
playerClaimCount[msg.sender] = playerClaimCount[msg.sender] + 1;
totalClaims = totalClaims + 1;
claimFee = claimFee + (claimFee * feeIncreasePercentage) / 100;
emit ThroneClaimed(
msg.sender,
sentAmount,
claimFee,
pot,
block.timestamp
);
}
Risk
Likelihood:
-
This will occur whenever a new player (not the current king) attempts to call claimThrone() during an active game round.
-
It will also occur during the first claim attempt after game deployment, since currentKing is initially address(0) and no one satisfies the condition.
Impact:
-
No new players can join the game, effectively halting the game after deployment or after the first claim.
-
The game becomes unusable and trust in the contract’s fairness is broken immediately.
Proof of Concept
Add this test to Game.t.sol and run with forge test --mt testOnlyCurrentKingCanClaimThrone
function testOnlyCurrentKingCanClaimThrone() public {
vm.prank(player1);
vm.expectRevert();
game.claimThrone{value: INITIAL_CLAIM_FEE}();
}
Recommended Mitigation
Fix the require statement to disallow the current King from claiming the throne:
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.");
uint256 sentAmount = msg.value;
uint256 previousKingPayout = 0;
uint256 currentPlatformFee = 0;
uint256 amountToPot = 0;
// Calculate platform fee
currentPlatformFee = (sentAmount * platformFeePercentage) / 100;
// Defensive check to ensure platformFee doesn't exceed available amount after previousKingPayout
if (currentPlatformFee > (sentAmount - previousKingPayout)) {
currentPlatformFee = sentAmount - previousKingPayout;
}
platformFeesBalance = platformFeesBalance + currentPlatformFee;
// Remaining amount goes to the pot
amountToPot = sentAmount - currentPlatformFee;
pot = pot + amountToPot;
// Update game state
currentKing = msg.sender;
lastClaimTime = block.timestamp;
playerClaimCount[msg.sender] = playerClaimCount[msg.sender] + 1;
totalClaims = totalClaims + 1;
// Increase the claim fee for the next player
claimFee = claimFee + (claimFee * feeIncreasePercentage) / 100;
emit ThroneClaimed(
msg.sender,
sentAmount,
claimFee,
pot,
block.timestamp
);
}
