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

Over-Restrictive `onlyOwner` on `withdrawPlatformFees()` Blocks Usability and Wastes Gas

theirrationalone

Root + Impact

Root Cause: Requiring the caller to be owner when the withdrawal always sends to owner -> Impact: Unnecessary access check, extra gas cost, and prevents anyone else from simply triggering the payout.

Description

  • The withdrawPlatformFees() function enforces an onlyOwner check even though its logic always sends the entire balance to the owner().

  • Since the recipient is fixed as the owner, requiring that the caller also be the owner adds no extra security but does incur a small gas overhead on each call. Furthermore, it prevents any benign helper contract or script from triggering the withdrawal on the owner’s behalf, forcing the owner’s own account to originate the transaction every time.

@> // @info: verbose authentication
// @explain: we're performing just pull or delegated pull then we would need
// only owner to call this function?
// @danger: GAS expensive and over restricted function
@> function withdrawPlatformFees() external onlyOwner nonReentrant {
uint256 amount = platformFeesBalance;
require(amount > 0, "Game: No platform fees to withdraw.");
​
platformFeesBalance = 0;
(bool success,) = payable(owner()).call{value: amount}("");
require(success, "Game: Failed to withdraw platform fees.");
​
emit PlatformFeesWithdrawn(owner(), amount);
}

Risk

Likelihood: High

  • onlyOwner check ensures msg.sender == owner().

  • Recipient is owner() regardless of who calls.

  • Issue: Caller restriction duplicates the recipient check and adds ~100–200 gas for the modifier’s storage read and comparison.

Impact: Gas

  • Minor Gas Overhead: Extra storage read and comparison per call (~100–200 gas).

  • Rigid UX: Only the owner’s own account can trigger withdrawal, preventing simple helpers or scripts from automating payouts.

  • No Security Gain: Caller restriction duplicates the transfer-to-owner guarantee already in the code.

Tools Used:

  • Foundry Test Suite

  • Chat-GPT AI Assistance (Report Grammar Check & Improvements)

  • Manual Review

Proof of Concept

Add to test/Game.t.sol:

function test_withdrawPlatformFees_overRestrictiveOnlyOwner() public {
vm.startPrank(player1);
game.claimThrone{value: INITIAL_CLAIM_FEE}();
vm.stopPrank();
​
uint256 platformFeeBalance = game.platformFeesBalance();
uint256 deployerBalanceBeforeWithdrawal = deployer.balance;
​
// 1) Non-owner call
vm.startPrank(player1);
vm.expectRevert(
abi.encodeWithSelector(bytes4(keccak256(abi.encodePacked("OwnableUnauthorizedAccount(address)"))), player1)
);
game.withdrawPlatformFees();
vm.stopPrank();
​
// 2) Owner call
vm.startPrank(deployer);
game.withdrawPlatformFees();
vm.stopPrank();
​
// Assert funds received by owner
assertEq(deployerBalanceBeforeWithdrawal + platformFeeBalance, deployer.balance);
}

Run:

forge test --mt test_withdrawPlatformFees_overRestrictiveOnlyOwner

If you remove onlyOwner, step (1) succeeds (any caller can trigger), step (2) still sends to owner, demonstrating identical security with greater flexibility.

Scenario:

A simple helper script or automated tool wants to trigger the owner’s fee withdrawal. Because onlyOwner is required, that script must use the owner’s private key rather than a simpler relay or proxy. Removing onlyOwner would allow any address to call the function, but funds still go only to owner().

Recommended Mitigation

Remove onlyOwner Modifier

- function withdrawPlatformFees() external onlyOwner nonReentrant {
+ function withdrawPlatformFees() external nonReentrant {
uint256 amount = platformFeesBalance;
require(amount > 0, "Game: No platform fees to withdraw.");
​
platformFeesBalance = 0;
​
(bool success,) = payable(owner()).call{value: amount}("");
require(success, "Game: Failed to withdraw platform fees.");
​
emit PlatformFeesWithdrawn(owner(), amount);
}

  • Any caller can now trigger the withdrawal, but funds still go only to owner().

Updates

Appeal created

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.