Use SafeERC20 instead of IERC20 in contracts/mocks/DummyRouter.sol

[code423n4]

Handle

fatima_naz

Vulnerability details

Impact

In line - 20 , 29 and 30. ERC20 transfer and transferFrom are used. The race condition that happens the most on the network today is the race condition in the ERC20 token standard.
The ERC20 token standard includes a function called 'approve' which allows an address to approve another address to spend tokens on their behalf. Assume that Alice has approved Eve to spend n of her tokens, then Alice decides to change Eve's approval to m tokens. Alice submits a function call to approve with the value n for Eve. Eve runs a Ethereum node so knows that Alice is going to change her approval to m. Eve then submits a tranferFrom request sending n of Alice's tokens to herself, but gives it a much higher gas price than Alice's transaction. The transferFrom executes first so gives Eve n tokens and sets Eve's approval to zero. Then Alice's transaction executes and sets Eve's approval to m. Eve then sends those m tokens to herself as well. Thus Eve gets n + m tokens even thought she should have gotten at most max(n,m).

Proof of Concept

https://swcregistry.io/docs/SWC-114
https://medium.com/coinmonks/solidity-transaction-ordering-attacks-1193a014884e

Tools Used

Recommended Mitigation Steps

use SafeERC20 - Library that brings an abstract layer above the ERC20 standard interface providing a way to call its methods safely by checking pre and post-conditions.

Add these 2 lines-
import "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";
using SafeERC20 for IERC20

Replace transferFrom with SafeTransferFrom
Replace transfer with SafeTransfer

[adrien-supizet]

This is a mock contract that will obviously not be used in production, and only for tests.

[alcueca]

Dispute rejected, scope wasn't set.