ak1 - MarginTradingFactory.sol : Contract does not user the SafeERC20, though it is inherited

[sherlock-admin]

ak1

medium

MarginTradingFactory.sol : Contract does not user the SafeERC20, though it is inherited

Summary

MarginTradingFactory.sol contract inherits the SafeERC20 from openzeppalin. I think, this has done to avoid any unexpected failures with ERC20 token transfer. But the SafeERC20 is not used.

The contract does some ERC20 token transfers.

Vulnerability Detail

Inherits the SafeERC20 in https://github.com/sherlock-audit/2023-05-dodo/blob/main/dodo-margin-trading-contracts/contracts/marginTrading/MarginTradingFactory.sol#L5

Uses the ERC20 transfer call here https://github.com/sherlock-audit/2023-05-dodo/blob/main/dodo-margin-trading-contracts/contracts/marginTrading/MarginTradingFactory.sol#L206-L218

Impact

Funds can be lost when contract can not accept the transaction. one scenario where this can happen this can happen due to out of gas run. When huge sum of transfer happens, failure at that stage could cause considerable amount of loss.

Code Snippet

https://github.com/sherlock-audit/2023-05-dodo/blob/main/dodo-margin-trading-contracts/contracts/marginTrading/MarginTradingFactory.sol#L206-L218

Tool used

Manual Review

Recommendation

Use the SafeERC20 as shown below.

contract MarginTradingFactory is Ownable, IMarginTradingFactory {
null(address _tokenAddress, address _to, uint256 _amount);

null(address _to, uint256 _amount);

using SafeERC20 for IERC20; ------------------------->>>>>>>>> add this line, and use the ERC20 typecase where token transfer happens

address public immutable MARGIN_TRADING_TEMPLATE;
address internal LendingPool;
IWETH internal WETH;
IDODOApprove internal DODOApprove;
// user => approveAddress = > bool
mapping(address => mapping(address => bool)) public ALLOWED_FLASH_LOAN;