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

The new claim fee is calculated from the previous claim fee in claimThrone function - Should be calculated from current claim fee, increased by a percentage

rjl

Root + Impact

Description

  • In the claimThrone method, the current contract calculates the new claimfee from the previous claimfee increased by a percentage, instead of calculating the new claim fee for the next king, with the actual claimFee from the new king (which is the sentAmount), increased by a percentage.

  • At the point that claimFee is used to calculate the new claim fee it has not yet been set to the actual current claim fee, it still corresponds to the previously set claim fee, as this is the first place the variable is set in the function.

  • Calculating the new claimfee in this way wouldn't ensure that each claim fee must be higher than the previous one. Instead, the new claim fee should be calculated based on what was just paid (the sentAmount) by the new king, increased by the percentage specified by the feeIncreasePercentage.

​
// 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
);
​
​

Risk

Likelihood:

  • This will occur when the claimThrone method is called, causing an incorrect calculation of the new claim fee that must be paid by the next king to claim the throne.


Impact:

  • Incorrect new claim fee calculations will be made, which could allow a player calling claimThrone to take the throne that has not paid the correct minimumum claim fee based on the claim fee that was paid by the current king. The current implementation would not even guarantee that a new king would pay a fee greater than what was paid by the current king.


Proof of Concept

The actual claim fee that was just paid by the new king corresponds to the value in sentAmount set in line 5 from msg.value.

As you can see in original current claimThrone function shown below, claimFee is set for the first time in line 30.

The true claim fee that was just paid by the current (new king) is stored in sentAmount.

In line 30 claimFee (the amount that must be at least paid by the next king) is being updated based on its current value, which has not been set yet to reflect the true new claimFee.

The variable claimFee at the point that it is being used to recalculate a new claim fee to be paid, still corresponds to the previous claim fee (or initial claim fee default) and not the current claim fee paid which is stored in sentAmount.

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;
​
// 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
);
}

Recommended Mitigation

Base the new claimFee (the minimum claim fee for the next player) on the current claim fee that was paid by the player that called claimThrone which is stored in sentAmount.

​
// Increase the claim fee for the next player
- claimFee = claimFee + (claimFee * feeIncreasePercentage) / 100;
+ claimFee = sentAmount + (sentAmount * feeIncreasePercentage) / 100;
​
- remove this code
+ add this code
Updates

Appeal created

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

Overpayment not refunded, included in pot, but not in claim fee

Support

FAQs

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

Give us feedback!