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

currentKing is not receiving any payment on the next claimt

neomartis

currentKing is not receiving any payment on the next claim

Description

  • As per description the king should"Receives a small payout from the next player's claimFee (if applicable)."

  • However, there is no logic in place that updates the pendingWinnings of the previous king

uint256 sentAmount = msg.value;
// The previousKingPayout is always set to 0 and not updated afterwards
@> 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;
​

Risk

Likelihood: High

  • This is a functionality that is part of the requirements but not implemented.

Impact: Medium

  • The king is expected to receive a small payout, but will never receive it

Proof of Concept

To validate the issue, we can run the following test (with the default setup as per Game.t.sol)
NOTE: that this requires to fix the issue in H1 first (to be able to claim the throne)

// Claim for the first time
vm.prank(player1);
game.claimThrone{value: INITIAL_CLAIM_FEE}();
assertEq(game.currentKing(), player1);
​
uint256 player1Balance = address(player1).balance;
​
// Claim for the second time
vm.prank(player2);
game.claimThrone{value: INITIAL_CLAIM_FEE + INITIAL_CLAIM_FEE * FEE_INCREASE_PERCENTAGE / 100}();
assertEq(game.currentKing(), player2);
​
// Finish the game
vm.warp(block.timestamp + GRACE_PERIOD + 1);
vm.prank(deployer);
game.declareWinner();
assertEq(game.gameEnded(), true);
​
// Withdraw winnings for player 1
// ISSUE: there is no winnings
vm.prank(player1);
vm.expectRevert("Game: No winnings to withdraw.");
game.withdrawWinnings();
​
​
uint256 player1BalanceAfterClaim = address(player1).balance;
​
// Player 1 should have received a fee, but that is not the case
console2.log("player1Balance", player1Balance);
console2.log("player1BalanceAfterClaim", player1BalanceAfterClaim);
assertEq(player1BalanceAfterClaim, player1Balance);

logs

[PASS] testIssueClaimWillNotReceivePaymentFromNextPlayer() (gas: 259722)
Logs:
player1Balance 9900000000000000000
player1BalanceAfterClaim 9900000000000000000

As can be seen from the logs and this test, we have player1 being the first king, then player2 claiming the throne afterwards. It is expected that player 1 will have earned a small fee. However the balance of player1 is the same before and after the game (and calling claimWinnings).

Instead withdrawWinnings called from player1 will revert.

Recommended Mitigation

In order to fix this, the logic needs to be built. There is currently only local uint256 previousKingPayout = 0; that is not used (it is not set and always 0). Roughly the following should be considered (as this is a missing feature, and no documentation on how much the amount should be):

  • Add logic to calculate the payout amount (either percentage based or fixed amount)

  • on a new claimThrone: update the pendingWinnings of the previous king (so currentKing before assigning it to the new caller)

  • then the payout could be claimed via withdrawWinnings

Updates

Appeal created

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

Missing Previous King Payout Functionality

Support

FAQs

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

Give us feedback!