Previous kings do not receive payouts after being overthrown.
Description: The README.md states that the king "receives a small payout from the next player's claimFee." This implies that when a new player claims the throne, a portion of the claimFee paid by the new king should be allocated to the previous king.
Under the current setup, there is no logic to calculate or distribute any payout to the previous king. The previous kings' address is also overwritten by the new king.
Risk:
Impact: Medium/High
Likelihood: High
Proof of Concept:
Player 1 claims the throne
Player 2 claims the throne
Warp time to end the game
Players 1 and 2 check payouts via pending winnings
Player 1 has no pending winnings despite being a previous king.
Proof of Code: Paste the following into Game.t.sol:
function test_PreviousKingReceivesHisPayout() public {
vm.startPrank(player1);
game.claimThrone{value: INITIAL_CLAIM_FEE}();
vm.stopPrank();
uint256 player1BalanceAfterClaim = player1.balance;
console2.log("Player 1 balance after claiming throne:", player1BalanceAfterClaim);
assertEq(game.currentKing(), player1, "Player 1 should be the throne holder");
assertEq(
player1BalanceAfterClaim, 10 ether - INITIAL_CLAIM_FEE, "Player 1 balance should be reduced by claim fee"
);
vm.startPrank(player2);
game.claimThrone{value: INITIAL_CLAIM_FEE + (INITIAL_CLAIM_FEE * FEE_INCREASE_PERCENTAGE) / 100}();
vm.stopPrank();
uint256 player2BalanceAfterClaim = player2.balance;
uint256 player1BalanceAfterPayout = player1.balance;
console2.log("Player 2 balance after claiming throne:", player2BalanceAfterClaim);
console2.log("Player 1 balance after receiving payout:", player1BalanceAfterPayout);
assertEq(game.currentKing(), player2, "Player 2 should be the throne holder");
assertEq(
player2BalanceAfterClaim,
10 ether - (INITIAL_CLAIM_FEE + (INITIAL_CLAIM_FEE * FEE_INCREASE_PERCENTAGE) / 100),
"Player 2 balance should be reduced by claim fee"
);
vm.warp(block.timestamp + GRACE_PERIOD + 1);
vm.startPrank(deployer);
game.declareWinner();
vm.stopPrank();
uint256 player1PendingWinnings = game.pendingWinnings(player1);
console2.log("Player 1 pending winnings:", player1PendingWinnings);
uint256 player2PendingWinnings = game.pendingWinnings(player2);
console2.log("Player 2 pending winnings:", player2PendingWinnings);
}
Here is the console.log output from running this test:
Logs:
Player 1 balance after claiming throne: 9900000000000000000
Player 2 balance after claiming throne: 9890000000000000000
Player 1 balance after receiving payout: 9900000000000000000
Player 1 pending winnings: 0
Player 2 pending winnings: 199500000000000000
You can see player 1 (the overthrown king) does not have any "pending winnings" at the conclusion of the game.
Recommended Mitigation: There are three things that need to be fixed. First, we need to declare a new public state variable. Second, we must define a "payout percentage" to the previous kings. Finally, we must update and calculate the previousKingPayout based on the current claimFee and transfer it to the previous king's pendingWinngins.
Here is how to add the public state variable at the top of the Game.sol contract:
+ uint256 public previousKingPayoutPercentage;
Here is how to edit the constructor:
constructor(
uint256 _initialClaimFee,
uint256 _gracePeriod,
uint256 _feeIncreasePercentage,
uint256 _platformFeePercentage
+ uint256 _previousKingPayoutPercentage
) Ownable(msg.sender) {
// Set deployer as owner
require(_initialClaimFee > 0, "Game: Initial claim fee must be greater than zero.");
require(_gracePeriod > 0, "Game: Grace period must be greater than zero.");
require(_feeIncreasePercentage <= 100, "Game: Fee increase percentage must be 0-100.");
require(_platformFeePercentage <= 100, "Game: Platform fee percentage must be 0-100.");
+ require(_previousKingPayoutPercentage <= 100, "Game: Platform fee percentage must be 0-100.");
initialClaimFee = _initialClaimFee;
initialGracePeriod = _gracePeriod;
feeIncreasePercentage = _feeIncreasePercentage;
platformFeePercentage = _platformFeePercentage;
+ previousKingPayoutPercentage = _previousKingPayoutPercentage
}
Here is how we should edit game::claimThrone():
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;
// Calculate platform fee
currentPlatformFee = (sentAmount * platformFeePercentage) / 100;
+ // Calculate previous king payout (for example - 10% of claimFee)
+ if (currentKing != address(0)) { // Ensure there is a previous king
+ previousKingPayout = (sentAmount * previousKingPayoutPercentage) / 100;
+ pendingWinnings[currentKing] = pendingWinnings[currentKing] + previousKingPayout;
+ }
// 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;
+ amountToPot = sentAmount - currentPlatformFee - previousKingPayout;
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);
}
This logic will allow the previousKingPayout to be stored in pendingWinnings, which can later be withdrawn using withdrawWinnings. This now correctly aligns with the documentation of the protocol.
