Last Man Standing
First Flight #45
Beginner FriendlyFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Logic Error in `game::claimThrone()` Prevents All Users from Claiming the Throne.

mustby

[H-1] Logic Error in game::claimThrone() Prevents All Users from Claiming the Throne.

Description: In a working protocol, users are able to call claimThrone() multiple times during the duration of a game to become king. In the current Game::claimThone() implementation, nobody is able to successfully become king. The "require(msg.sender == currentKing,...) breaks the intended flow and functionality of the entire protocol. Please see the code below:

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.");

Impact: HIGH

  • Protocol functionality is totally destroyed

Likelihood: HIGH

  • The successful calling of Game::claimThrone() is an integral part of the entire "Last Man Standing" protocol. This would likely be called mutliple times per game.

Proof of Concept:

  1. Prank player 1.

  2. Call Game::claimThrone with the required INITIAL_CLAIM_FEE.

  3. Compare the address of the current king to the address of player 1.

  4. Run the console2.log of the current king to see that the king address stays as address zero.

Proof of Code: Paste the below code in Game.t.sol:

function test_CanPlayerCallClaimThrone() public {
vm.startPrank(player1);
vm.expectRevert("Game: You are already the king. No need to re-claim.");
game.claimThrone{value: INITIAL_CLAIM_FEE}();
vm.stopPrank();
assertEq(game.currentKing(), player1, "Player 1 should be the throne holder");
console2.log(game.currentKing());
}

This test will pass becasue we are expecting the revert due to the logic error in the second require line of the function.

Recommended Mitigation: I would recommend changing the second require line in Game::claimThrone() to "!=" instead of "==". This would allow users who are NOT the current king to call the function. This would align with the protocol's intended functionality.

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.");
Updates

Appeal created

inallhonesty Lead Judge about 2 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Game::claimThrone `msg.sender == currentKing` check is busted

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.