Root + Impact
Description
The Game::claimThrone function intent is to allow new players to claim the throne before grace period elapse, however the function contains a check which disables people from participating in contest, stating the msg.sender must be currentKing , before now currentKing is address(0), this line prevents anyone from participating and breaks the protocol invariants
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:
Impact:
Proof of Concept
A player enters the game by calling Game::claimThrone , the participant gets a revert error because they are not the currentKing
function testNobodyCanClaimThrone() public {
vm.prank(player1);
vm.expectRevert('Game: You are already the king. No need to re-claim.');
game.claimThrone{value: 1 ether}();
}
Result:
Ran 1 test for test/Game.t.sol:GameTest
[PASS] testNobodyCanClaimThrone() (gas: 47025)
Traces:
[47025] GameTest::testNobodyCanClaimThrone()
├─ [0] VM::prank(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84])
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error 0xf28dceb3: 4Game: You are already the king. No need to re-claim.)
│ └─ ← [Return]
├─ [29259] Game::claimThrone{value: 1000000000000000000}()
│ └─ ← [Revert] Game: You are already the king. No need to re-claim.
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 19.82ms (2.83ms CPU time)
Recommended Mitigation
The require statement in Game::claimThrone should check msg.sender is not currentKing, this way it ensures currentKing won't reclaim throne until another player claims 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.");
+ 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;
// 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
);
}
