Incorrect Validation Logic which allows only the current king to reclaim the throne, thereby blocking other players from being able to claim it.
Description
Game::claimThrone function has a require check to ensure that when a player is the currentKing, they cannot reclaim the throne. Instead, it is set up to allow the currentKing be the player to claim and reclaim the throne, thereby preventing other players 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.");
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: HIGH
Impact: HIGH
-
This will lead to the failure of one of the protocol's core purposes, which is for players to compete for the throne. Players will stop participating
-
It will also result in revenue loss, as the protocol will be unable to collect claimFee from new participants and platform fees.
Proof of Concept
player1 claims the throne and becomes the current king.
player1 is checked against game::currentKing() and it is confirmed that player1 is now the currentKing.
player2 tries to claim the throne but is prevented, which leads to a revert.
function testClaimThroneHasWrongRequireCheckForCurrentKing() public {
vm.prank(player1);
game.claimThrone{value: INITIAL_CLAIM_FEE}();
assertEq(game.currentKing(), player1);
vm.expectRevert("Game: You are already the king. No need to re-claim.");
vm.prank(player2);
game.claimThrone{value: game.claimFee()}();
}
Recommended Mitigation
To prevent this, ensure that the correct condition check is implemented i.e msg.sender should not be equal to currentKing.
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;
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);
}
