Submission Details
Severity:
Discrepancy between NatSpec and implementation related to `previousKingPayout` in `claimThrone` function
Summary
The NatSpec statements in the claimThrone function indicates that a portion of the new claim fee should be sent to the previous king. However, the implementation does not reflect this behavior. The previousKingPayout variable is initialized to zero and remains unchanged throughout the function, resulting in no payout being made.
Description
Natspec in claimThrone function states that If there's a previous king, a small portion of the new claim fee is sent to them.. However, there was no code within the function that did such accounting. The previousKingPayout was initialized as zero and remained this value throughout the function call.
/**
* @dev Allows a player to claim the throne by sending the required claim fee.
<@@> * If there's a previous king, a small portion of the new claim fee is sent to them.
* A portion also goes to the platform owner, and the rest adds to the pot.
*/
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;
<@@> //*** no code implementation about previousKingPayout!!
//`previousKingPayout` in the if statement below is still zero in value
// Defensive check to ensure platformFee doesn't exceed available amount after previousKingPayout
if (currentPlatformFee > (sentAmount - previousKingPayout)) {
currentPlatformFee = sentAmount - previousKingPayout;
}
platformFeesBalance = platformFeesBalance + currentPlatformFee;
...
}
Risk
Likelihood:
This issue will consistently occur after the first claim, as every subsequent call to the
claimThronefunction fails to distribute a portion of the new claim fee to the previous king.
Impact:
The previous king receives no payout from the new claim fee, undermining the intended incentive mechanism and potentially discouraging continued participation.
Proof of Concept
In test/Game.t.sol, add the following test:
function test_audit_missingPreviousKingPayout() public {
// prerequisite: change to `require(msg.sender != currentKing, "Game: You are already the king. No need to re-claim.");` in `claimThrone` function before the test
// player1 being the first claimer
vm.prank(player1);
game.claimThrone{value: INITIAL_CLAIM_FEE}();
assertEq(game.currentKing(), player1);
uint256 player1_balance_before_player2Claim = player1.balance;
console2.log("player1_balance_before_player2Claim: ", player1_balance_before_player2Claim);
// player2 as 2nd claimer after player1
vm.prank(player2);
game.claimThrone{value: INITIAL_CLAIM_FEE * 110/100} ();
assertEq(game.currentKing(), player2);
// to check player1 balance after player2 called claimThrone
uint256 player1_balance_after_player2Claim = player1.balance;
console2.log("player1_balance_after_player2Claim: ", player1_balance_after_player2Claim);
// passing the assertion below means that player1 balance was found remained the same even after player 2 had claimed
// and became the currentKing, indicating that no payout was received by player 1 as the previousKingPayout
assertEq(player1_balance_before_player2Claim, player1_balance_after_player2Claim);
}
In terminal, run forge test --match-test test_audit_missingPreviousKingPayout -vvv will generate the following results:
forge test --match-test test_audit_missingPreviousKingPayout -vvv [10:24:05]
...
Ran 1 test for test/Game.t.sol:GameTest
[PASS] test_audit_missingPreviousKingPayout() (gas: 198427)
Logs:
player1_balance_before_player2Claim: 9900000000000000000
player1_balance_after_player2Claim: 9900000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 767.17µs (95.08µs CPU time)
Ran 1 test suite in 115.37ms (767.17µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
The test passed indicating that no payout was received by player1 as the previous king from player2 even after player2 called claimThrone and became the currentKing.
Recommended Mitigation
Factor in the calculation for previousKingPayout and implement it in the claimThrone function. The payout could ideally be a percentage of the new claim fee. Update the corresponding changes as proposed below:
contract Game is Ownable {
// --- State Variables ---
...
// Game Parameters (Configurable by Owner)
uint256 public initialClaimFee; // The starting fee for a new game round
uint256 public feeIncreasePercentage; // Percentage by which the claimFee increases after each successful claim (e.g., 10 for 10%)
uint256 public platformFeePercentage; // Percentage of the claimFee that goes to the contract owner (deployer)
+ uint256 public previousKingFeePercentage;
...
constructor(
uint256 _initialClaimFee,
uint256 _gracePeriod,
uint256 _feeIncreasePercentage,
uint256 _platformFeePercentage,
+ uint256 _previousKingFeePercentage
) Ownable(msg.sender) { // Set deployer as owner
require(_initialClaimFee > 0, "Game: Initial claim fee must be greater than zero.");
require(_gracePeriod > 0, "Game: Grace period must be greater than zero.");
require(_feeIncreasePercentage <= 100, "Game: Fee increase percentage must be 0-100.");
require(_platformFeePercentage <= 100, "Game: Platform fee percentage must be 0-100.");
+ require(_previousKingFeePercentage <= 100, "Game: Previous king fee percentage must be 0-100.");
initialClaimFee = _initialClaimFee;
initialGracePeriod = _gracePeriod;
feeIncreasePercentage = _feeIncreasePercentage;
platformFeePercentage = _platformFeePercentage;
+ previousKingFeePercentage = _previousKingFeePercentage;
...
}
...
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;
+ // To implement project intended payout scheme to previous king
+ // with previousKingFeePercentage as global state variable defined via constructor
+ // and later on adjustable by owner under updateClaimFeeParameters
+ if (currentKing != address(0)) {
+ previousKingPayout = (sentAmount * previousKingFeePercentage) / 100;
+ // could also implement as fix percentage if don't want to create new global state variable
+ // previousKingPayout = (sentAmount * 10) / 100;
+ (bool success, ) = currentKing.call{value: previousKingPayout}("");
+ require(success, "Game: Failed to pay previous king.");
+ }
// 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;
+ amountToPot = sentAmount - currentPlatformFee - previousKingPayout;
pot = pot + amountToPot;
...
}
function updateClaimFeeParameters(
uint256 _newInitialClaimFee,
uint256 _newFeeIncreasePercentage
+ uint256 _newPreviousKingFeePercentage
) external onlyOwner isValidPercentage(_newFeeIncreasePercentage) {
require(_newInitialClaimFee > 0, "Game: New initial claim fee must be greater than zero.");
initialClaimFee = _newInitialClaimFee;
feeIncreasePercentage = _newFeeIncreasePercentage;
+ previousKingFeePercentage = _newPreviousKingFeePercentage
- emit ClaimFeeParametersUpdated(_newInitialClaimFee, _newFeeIncreasePercentage);
+ emit ClaimFeeParametersUpdated(_newInitialClaimFee, _newFeeIncreasePercentage, _newPreviousKingFeePercentage);
}
}
Updates
Appeal created
inallhonesty about 2 months ago
Submission Judgement Published Assigned finding tags:
Missing Previous King Payout Functionality
Rewards breakdown
nSLOC
181This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.