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

Non-Payable Contract Winner Locks Funds

ark845612

Description

  • Winners should receive ETH regardless of being EOA or contract addresses

  • Contracts without receive()/fallback() cause irreversible fund locking due to failed transfers

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender];
require(amount > 0, "Game: No winnings to withdraw.");
​
@> (bool success,) = payable(msg.sender).call{value: amount}("");
@> require(success, "Game: Failed to withdraw winnings."); // Hard revert
pendingWinnings[msg.sender] = 0;
}

Risk

Likelihood:

  • Occurs when any contract wins without payable fallback

  • Guaranteed failure for non-payable winner contracts

Impact:

  • Permanent ETH lock in contract

  • Broken game state requiring admin intervention

  • Winners lose rightful prizes

Proof of Concept

  1. Creates a contract winner (NoReceiveContract) that cannot receive ETH (no receive()/fallback())

  2. Makes this contract win the game legitimately through claimThrone() and declareWinner()

  3. Attempts to withdraw winnings to the non-payable contract

  4. Confirms:

    • Withdrawal reverts with "Failed to withdraw winnings"

    • Funds remain locked in pendingWinnings mapping

    • Exact expected winnings amount is preserved (after platform fee deduction)

function test_FundsLockedWhenWinnerCantReceiveETH() public {
// Setup malicious winner
NoReceiveContract badWinner = new NoReceiveContract(game);
vm.deal(address(badWinner), 10 ether);
uint256 claimFee = game.claimFee();
uint256 expectedWinnings = claimFee - ((claimFee * game.platformFeePercentage()) / 100);
​
// Win game legitimately
badWinner.attack(); // Calls claimThrone()
vm.warp(block.timestamp + game.gracePeriod() + 1);
game.declareWinner();
​
// Verify withdrawal fails
vm.expectRevert("Game: Failed to withdraw winnings.");
vm.prank(address(badWinner));
game.withdrawWinnings();
​
// Funds remain locked
assertEq(game.pendingWinnings(address(badWinner)), expectedWinnings);
}
​
contract NoReceiveContract {
Game game;
​
constructor(Game _game) {
game = _game;
}
​
function attack() public {
game.claimThrone{value: game.claimFee()}();
}
// No receive() or fallback()
}

Logs

forge test --mt test_FundsLockedWhenWinnerCantReceiveETH -vvvv
[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/Game.t.sol:GameTest
[PASS] test_FundsLockedWhenWinnerCantReceiveETH() (gas: 367435)
Traces:
[407235] GameTest::test_FundsLockedWhenWinnerCantReceiveETH()
├─ [117098] → new NoReceiveContract@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
│ └─ ← [Return] 472 bytes of code
├─ [0] VM::deal(NoReceiveContract: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 10000000000000000000 [1e19])
│ └─ ← [Return]
├─ [2514] Game::claimFee() [staticcall]
│ └─ ← [Return] 100000000000000000 [1e17]
├─ [2536] Game::platformFeePercentage() [staticcall]
│ └─ ← [Return] 5
├─ [157033] NoReceiveContract::attack()
│ ├─ [514] Game::claimFee() [staticcall]
│ │ └─ ← [Return] 100000000000000000 [1e17]
│ ├─ [148601] Game::claimThrone{value: 100000000000000000}()
│ │ ├─ emit ThroneClaimed(newKing: NoReceiveContract: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], claimAmount: 100000000000000000 [1e17], newClaimFee: 110000000000000000 [1.1e17], newPot: 95000000000000000 [9.5e16], timestamp: 1)
│ │ └─ ← [Stop]
│ └─ ← [Stop]
├─ [2536] Game::gracePeriod() [staticcall]
│ └─ ← [Return] 86400 [8.64e4]
├─ [0] VM::warp(86402 [8.64e4])
│ └─ ← [Return]
├─ [48689] Game::declareWinner()
│ ├─ emit GameEnded(winner: NoReceiveContract: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], prizeAmount: 0, timestamp: 86402 [8.64e4], round: 1)
│ └─ ← [Stop]
├─ [0] VM::expectRevert(custom error 0xf28dceb3: "Game: Failed to withdraw winnings.)
│ └─ ← [Return]
├─ [0] VM::prank(NoReceiveContract: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f])
│ └─ ← [Return]
├─ [28219] Game::withdrawWinnings()
│ ├─ [44] NoReceiveContract::fallback{value: 95000000000000000}()
│ │ └─ ← [Revert] EvmError: Revert
│ └─ ← [Revert] Game: Failed to withdraw winnings.
├─ [848] Game::pendingWinnings(NoReceiveContract: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
│ └─ ← [Return] 95000000000000000 [9.5e16]
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.12ms (212.92µs CPU time)
​
Ran 1 test suite in 5.18ms (1.12ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

function withdrawWinnings() external nonReentrant {
uint256 amount = pendingWinnings[msg.sender];
require(amount > 0, "No winnings");
+ pendingWinnings[msg.sender] = 0; // State change before transfer
(bool success,) = payable(msg.sender).call{value: amount}("");
- require(success, "Game: Failed to withdraw winnings.");
+ if (!success) {
+ pendingWinnings[msg.sender] = amount; // Revert state
+ revert("Transfer failed");
+ }
​
- pendingWinnings[msg.sender] = 0;
emit WinningsWithdrawn(msg.sender, amount);
}

  1. State Change Before Transfer: CEI pattern

  2. Gas Limit: Prevents gas-based DoS

  3. State Recovery: Reverts mapping on failure

  4. Clearer Errors: Specific revert reasons

Updates

Appeal created

inallhonesty Lead Judge about 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Doss by reverting contract / contract that doesn't implement receive.

The only party affected is the "attacker". Info at best.

Support

FAQs

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