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 4 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.

Give us feedback!