Root + Impact
Description
-
The expected behavior is that the game rewards the previous king with a payout when a new player claims the throne, before updating the current king. This ensures fairness and proper fund distribution.
-
The actual implementation introduces a variable previousKingPayout but sets it to 0 every time without updating it or transferring any funds to the previous king. This makes the payout logic non-functional and misleading in the code. Additionally, the conditional check using previousKingPayout is dead code, creating a false sense of security.
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:
Every time a player calls claimThrone(), the logic for paying the previous king is skipped because previousKingPayout is always zero.
The payout code path is dead, so the bug consistently occurs for all transactions.
Impact:
Previous king never receives a payout, breaking core game mechanics and fairness.
Users may lose trust in the platform, potentially causing financial and reputational damage.
Proof of Concept
The claimThrone() function initializes previousKingPayout to 0 every time and never updates it with a real payout amount.
There is no logic to transfer any funds to the previous king, even though the defensive check suggests such a mechanism exists.
function testClaimThrone_NoPayoutToPreviousKing() public {
vm.deal(playerA, 1 ether);
vm.prank(playerA);
game.claimThrone{value: 1 ether}();
vm.deal(playerB, 2 ether);
vm.prank(playerB);
game.claimThrone{value: 2 ether}();
assertEq(playerA.balance, 0 ether);
}
Recommended Mitigation
- uint256 previousKingPayout = 0; // Remove hardcoded zero
- if (currentPlatformFee > (sentAmount - previousKingPayout)) {
- currentPlatformFee = sentAmount - previousKingPayout;
- }
+ // Calculate payout for previous king before updating the state
+ uint256 previousKingPayout = (sentAmount * previousKingSharePercentage) / 100;
+ address payable oldKing = payable(currentKing);
+ if (previousKingPayout > 0 && oldKing != address(0)) {
+ (bool success, ) = oldKing.call{value: previousKingPayout}("");
+ require(success, "Game: Previous king payout failed");
+ }
+ // Ensure platform fee does not exceed remaining amount after previousKingPayout
+ currentPlatformFee = (sentAmount * platformFeePercentage) / 100;
+ if (currentPlatformFee > (sentAmount - previousKingPayout)) {
+ currentPlatformFee = sentAmount - previousKingPayout;
+ }
