Missing Logic For Previous King Payout Leads to The Previous King Never Being Paid Out
Description
-
Under the intended rules, when a player claims the throne, the previous king should receive a small portion of the claim fee as a payout. This incentivizes players to compete and rewards the dethroned king for holding the position.
-
However, while the variable previousKingPayout is declared in claimThrone(), it is never assigned a value or credited to the actual previous king. As a result, dethroned kings receive no payout at all, which violates game rules, misleads participants, and breaks part of the economic model.
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 every time a new player successfully calls claimThrone() after a previous king is in place.
-
It will continue indefinitely across all game rounds, since no logic ever pays the dethroned king.
Impact:
-
Players are financially disincentivized from participating, since there is no reward for holding the throne.
-
The core game loop is economically broken, reducing player engagement and eroding trust in the contract’s fairness.
Proof of Concept
Add this test to Game.t.sol and run with forge test --mt testPreviousKingDoesNotRecievePayout
function testPreviousKingDoesNotRecievePayout() public {
vm.startPrank(player1);
game.claimThrone{value: game.claimFee()}();
vm.stopPrank();
vm.startPrank(player2);
game.claimThrone{value: game.claimFee()}();
vm.stopPrank();
assertEq(game.pendingWinnings(player1), 0);
}
Recommended Mitigation
Create logic to calculate what the previousKingPayout will be set to every time someone claims the throne, and add a check that updates the pending winnings of the previous king:
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 previousKingPayout = /* logic here */;
uint256 currentPlatformFee = 0;
uint256 amountToPot = 0;
+ if (currentKing != address(0)) {
+ pendingWinnings[currentKing] += currentKingPayout;
+ }
// 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
);
}
