Root + Impact
Description
The Last Man Standing game is a game where each participant pays a fee to claim the throne. When someone become king, he has to give a portion of his fees as a payout for the previous king.
In the claimThrone function, there is a portion of the code that is useless because this functionnality has never been implemented.
function claimThrone() external payable gameNotEnded nonReentrant {
...
uint256 previousKingPayout = 0;
...
if (currentPlatformFee > (sentAmount - previousKingPayout)) {
currentPlatformFee = sentAmount - previousKingPayout;
}
}
This check ensure that platform fee doesn't exceed available amount after the payout of the previous king, otherwise it could cause an issue about the smart contract liquidity (if the payout was implemented of course).
But the state of the variable previousKingPayout never change before the condition, so the condition is always false, as currentPlatformFee is equal to :
currentPlatformFee = (sentAmount * platformFeePercentage) / 100;
In short, the current game doesn't offer a payout for the outgoing king, as cruel as it may be :(
Risk
Likelihood:
Impact:
Proof of Concept
No proof of concept
Recommended Mitigation
The recommandation could wether to update the contract so that we can offer a payout for the previous king, or just remove that piece of code (much easier).
We can implement the payout by changing the name of the variable pendingWinning for pendingRewards. This variable will handle all payouts that need to be redeemed, whether it's the winner pot or previous king payout.
+ mapping(address => uint256) public pendingReward;
- mapping(address => uint256) public pendingWinnings;
We have to add a new contract attribute to set the previous king payout percentage, that we need to initialize in the constructor. The percentage can be between 0 and 100 :
-
0% assuming that the previous king will not have any payout, all of the pot for the king !
-
100% assuming that the previous king will get back his payout. In this game, only the throne matter !
uint256 public payoutFeePercentage;
...
constructor(
uint256 _initialClaimFee,
uint256 _gracePeriod,
uint256 _feeIncreasePercentage,
uint256 _platformFeePercentage,
@> uint256 _payoutFeePercentage
) Ownable(msg.sender) {
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(
_payoutFeePercentage <= 100,
"Game: Platform fee percentage must be 0-100."
);
initialClaimFee = _initialClaimFee;
initialGracePeriod = _gracePeriod;
feeIncreasePercentage = _feeIncreasePercentage;
platformFeePercentage = _platformFeePercentage;
@> payoutFeePercentage = _payoutFeePercentage;
Then, we need to review the claimThrone function. To handle the payout easier, we will take it directly from the claim fee of the new king, as we do for the platform fee.
We first deduce platform fee, so that the percentage of the payout is calculated with the remaining claim fee. This has 2 benefits :
function claimThrone() public 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;
address previousKing = currentKing;
uint256 currentPlatformFee = 0;
uint256 amountToPot = 0;
currentPlatformFee = (sentAmount * platformFeePercentage) / 100;
platformFeesBalance = platformFeesBalance + currentPlatformFee;
@> if (previousKing != address(0)) {
@> previousKingPayout =
@> ((sentAmount - currentPlatformFee) * payoutFeePercentage) /
@> 100;
@> pendingRewards[previousKing] = pendingRewards[previousKing] + previousKingPayout;
@> }
amountToPot = sentAmount - currentPlatformFee - previousKingPayout;
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
);
}
Finally, we just have to rename the withdraw function, as the claim of the rewards is already functionnal :
function withdrawRewards() external nonReentrant {
uint256 amount = pendingRewards[msg.sender];
require(amount > 0, "Game: No rewards to withdraw.");
(bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Game: Failed to withdraw rewards.");
pendingRewards[msg.sender] = 0;
emit RewardsWithdrawn(msg.sender, amount);
}
Here is a test to verify that all work well :
uint256 public constant PAYOUT_FEE_PERCENTAGE = 50;
...
function testClaimThroneAndGetRewardsWhenOut() public {
vm.prank(player1);
game.claimThrone{value: 1 ether}();
assertEq(player1, game.currentKing());
vm.prank(player2);
game.claimThrone{value: 2 ether}();
console2.log("Pot", game.pot());
console2.log("Platform fees", game.platformFeesBalance());
console2.log("Pending reward", game.pendingRewards(player1));
vm.prank(player1);
game.withdrawRewards();
console2.log(player1.balance);
}
