search

sherlock-audit / 2023-05-dodo-judging

6 stars 2 forks source link

Quantish - Consider using `safeTransfer` instead of `transfer` #181

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Quantish

medium

Consider using safeTransfer instead of transfer

Summary

Consider using safeTransfer instead of transfer

Vulnerability Detail

In MarginTrading you use ERC20.transfer for transferring funds: https://github.com/sherlock-audit/2023-05-dodo/blob/main/dodo-margin-trading-contracts/contracts/marginTrading/MarginTrading.sol#L212 https://github.com/sherlock-audit/2023-05-dodo/blob/main/dodo-margin-trading-contracts/contracts/marginTrading/MarginTrading.sol#L341

Some of ERC20 tokens may not revert on unsuccessful transfer, but just return false instead: https://github.com/d-xo/weird-erc20/#no-revert-on-failure

Impact

If the MarginTrading contract is managed not by EOA but by another contract, such "silent fails" would make a mess and may lead to calculation errors in the owner contract, when it "withdraws" funds and sees that the tx is successful (but the funds are actually not sent because of some error).

Code Snippet

Tool used

Manual Review

Recommendation

Consider using SafeERC20.safeTransfer function instead.