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

External call `transferFrom` made before a state change, making it vulnerable to re-entrancy attacks

0xdef4

Summary

In joinGameWithToken, an external call to winningToken is made before a state change, leading to a possible re-entrancy attack.

Vulnerability Details

In joinGameWithToken, transferFrom is called before a state change to game.playerB,

function joinGameWithToken(uint256 _gameId) external {
.
.
.
winningToken.transferFrom(msg.sender, address(this), 1);
​
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
}

Although the winningToken is owned by the admin(owner) and is generally not considered a malicious contract in this case, if the token contract does have a malicious transferFrom function then it can lead to a possible re-entrancy attack.

For example,

contract MaliciousWinningToken {
function transferFrom(address from, address to, uint256 _amount) public returns (bool) {
RockPaperScissors(to).joinGameWithToken(1);
return true;
}
}

Impact

If the transferFrom function in a winningToken contract is malicious, it can re-enter by using joinGameWithToken causing in infinite loop.

Tools Used

VSCode

Recommendations

It is best pract to follow CEI pattern, and add a check to see if playerB already joined.

function joinGameWithToken(uint256 _gameId) external {
Game storage game = games[_gameId];
​
require(game.state == GameState.Created, "Game not open to join");
require(game.playerA != msg.sender, "Cannot join your own game");
require(block.timestamp <= game.joinDeadline, "Join deadline passed");
require(game.bet == 0, "This game requires ETH bet");
require(winningToken.balanceOf(msg.sender) >= 1, "Must have winning token");
+ require(game.playerB == address(0), "Player B already joined");
​
- winningToken.transferFrom(msg.sender, address(this), 1);
game.playerB = msg.sender; // 상태 변경 먼저
emit PlayerJoined(_gameId, msg.sender);
​
+ winningToken.transferFrom(msg.sender, address(this), 1);
}
Updates

Appeal created

m3dython Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Too generic
m3dython Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Too generic

Support

FAQs

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