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

Use `safeTransferFrom` instead of `transferFrom`

ladiknemcem

Summary:

In `RockPaperScissors.sol` contract in function createGameWithToken you are using transferFrom instead of safeTransferFrom.

function createGameWithToken(
uint256 _totalTurns,
uint256 _timeoutInterval
) external returns (uint256) {
require(
winningToken.balanceOf(msg.sender) >= 1,
"Must have winning token"
);
require(_totalTurns > 0, "Must have at least one turn");
require(_totalTurns % 2 == 1, "Total turns must be odd");
require(
_timeoutInterval >= 5 minutes,
"Timeout must be at least 5 minutes"
);
​
// Transfer token to contract
winningToken.transferFrom(msg.sender, address(this), 1); // @audit-issue Use safeTransferFrom.
​
// code
}

TransferFrom does not revert on failure. This means if the transferFrom fails, the function will continue like nothing happened, without a revert.

Vulnerability Details:

Even though you are checking if the msg.sender has the winningToken before executing transferFrom.

This is not enough, because the msg.sender could be a smart contract which did not approve his winningToken to you. This means the transferFrom will fail due to insufficient approval.

Same for function `joinGameWithToken`.

Impact:

Users are able to call `createGameWithToken` without actually having the token.

Tools Used:

Manual review

Recommendations:

Use safeTransferFrom from Openzeppelin SafeERC20 library.

Updates

Appeal created

m3dython Lead Judge about 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Usage of safeTransferFrom

WinningToken inherits OpenZeppelin's standard ERC20 implementation, where transferFrom already reverts on insufficient allowance or balance

m3dython Lead Judge about 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Usage of safeTransferFrom

WinningToken inherits OpenZeppelin's standard ERC20 implementation, where transferFrom already reverts on insufficient allowance or balance

Support

FAQs

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