Inappropriate function visibility

Low ML – 01 |  Resolved

OwnableUpgradeable smart contract is a modified version of openzeppelin standard contract (renounceOwnership() function removed). There is transferOwnership() function that is not called in the contract itself so its visibility can be improved from public to external.

Recommendation:

Consider changing visibility from public to external to safe gas usage on calling this function.

Usage of msg.sender

Low ML – 02 |  Resolved

In EGMigrate smart contract could be used _msgSender() function instead of msg.sender because it is inherited from ContextUpgradeable smart contract. In addition, the _msgSender() function call requires less gas than msg.sender.

Recommendation:

Consider changing msg.sender to _msgSender().

Too much import code

Low ML – 03 |  Resolved

Imports of IERC20 and ERC20 could be replaced by import of IERC20Metadata that already includes the IERC20 interface and does not contain unnecessary functionality of ERC20 smart contract.

Recommendation:

Consider changing imports:

  import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
  import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

to:

  import "@openzeppelin/contracts/token/ERC20/extensions/
  IERC20Metadata.sol";

Also change ERC20 type usage to IERC20Metadata for decimals() calls.

Incorrect address stored during migration

Low ML – 04 |  Resolved

In migrate() function of EGMigrate smart contract there is storing Migration to _userMigrations of _msgSender() but toAddress is not stored:

  Migration({
  migrationId: migrationCounter,
  toAddress: _msgSender(), //! should be toAddress input variable
  timestamp: block.timestamp,
  amountOfSourceToken: amount,
  amountOfTargetToken: migrationAmount
  })

Recommendation:

Change toAddress: _msgSender() to toAddress: toAddress.

Lack of require statements

Low TL – 01 |  Resolved

The functions addMigrationToken() and updateMigrationTokenInfo() are not provided with input parameters validation of sourceToken and targetToken parameters, can be passed the same addresses for source and target tokens.

Recommendation:

Consider adding next require statement (require(sourceToken != targetToken)).

Lack of transfer amount validation

Low TL – 02 |  Resolved

The function returnTokens() isn’t provided with validation amount tokens to transfer, contract balance of target token can be less than the passed amount which will lead to an error in transfer function.

Recommendation:

Consider adding require for check contract balance of target token (require(amount => IERC20(migrationToken.targetToken).balanceOf(address(this))).

Solidity source file contains two contract definitions

Informational  MI – 01 |  Resolved

Usually, one source file contains one smart contract. OwnableUpgradeable smart contract implements a module of certain functionality and can be placed in an independent source file.

Recommendation:

Consider moving OwnableUpgradeable smart contract to separate source file.

Missed NatSpec

Informational  MI – 02 |  Resolved

NatSpec comments are missed before EGMigrate smart contract declaration and initialize() function. A public state variable is equivalent to a function for the purposes of NatSpec. Events also need NatSpec notations.

Recommendation:

Consider adding NatSpec for EGMigrate SC, initialize() function and events. Also, change comments of public variables to NatSpec comments. View NatSpec Format — Solidity 0.8.17 documentation.

Private variable naming style

Informational  MI – 03 |  Resolved

Private and internal state variable names should use underscore and mixedCase (_initialSupply, _account, _recipientAddress etc).View Style Guide — Solidity 0.8.17 documentation.

Recommendation:

Consider changing userMigrations to _userMigrations.

‍