Last Man Standing
First Flight #45
Beginner FriendlyFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: medium
Likelihood: medium
Invalid

Misleading Function Name and Missing Access Control in withdrawWinnings()

seenu1947

Misleading Function Name and Missing Access Control in withdrawWinnings()

Description

  • The function withdrawWinnings() is documented to be exclusively for the “declared winner” to withdraw their prize. This implies that only the winner of the game should be able to call this function.

  • However, the actual implementation allows any address with a non-zero balance in pendingWinnings to call the function and withdraw funds, regardless of whether they are the declared winner. There is no check to restrict it to the current winner or validate game state.

/**
* @dev Allows the declared winner to withdraw their prize.
* Uses a secure withdraw pattern with a manual reentrancy guard.
*/
function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender]; @> Anyone with non-zero balance can withdraw
require(amount > 0, "Game: No winnings to withdraw.");
​
(bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Game: Failed to withdraw winnings.");
​
pendingWinnings[msg.sender] = 0;
​
emit WinningsWithdrawn(msg.sender, amount);
}

Risk

Likelihood:

  • The misleading name and docstring could lead future maintainers to incorrectly assume this function is secure and restricted.

  • Any user or smart contract that accumulates a balance in pendingWinnings — intentionally or by accident — can withdraw funds using this function.

Impact:

  • If pendingWinnings gets misused or misassigned due to a bug elsewhere, any address can withdraw ETH.

  • May result in unexpected fund distribution, or user trust issues due to misleading documentation.

Proof of Concept

/**
* @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.");
​

Recommended Mitigation

Fix the naming and documentation
If this function is meant to allow any player to withdraw their pending rewards (e.g., partial refunds, pot shares), then:
- /// @dev Allows the declared winner to withdraw their prize.
+ /// @dev Allows any player to withdraw their pending balance.
function withdrawWinnings() external nonReentrant {
​
OR
​
Restrict access only to the winner
If it’s only for the winner to claim their pot:
​
function withdrawWinnings() external nonReentrant {
+ require(msg.sender == currentKing, "Game: Only the winner can withdraw the prize.");
​
Updates

Appeal created

inallhonesty Lead Judge about 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
seenu1947 Submitter
about 1 month ago
inallhonesty Lead Judge
about 1 month ago
inallhonesty Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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