When transferring tokens via
transfer(), several security concerns must be addressed.
First and foremost, the transfer status must be checked. This case is described in many places, while the most known is the
safeTransfer() implementation by OpenZeppelin, whose core function can be found
here.
ERC20 standard dictates the obligatory return of a success/failure value. However, non-standard tokens, such as USDT/BNB/etc, may not return a value at all (We don't include really "sick" tokens, i.e., returning false on successful transfer). To handle this, the "safe" transfer return value check can be described as: "the return value is optional (in rare cases), but if data is returned, it must not be false".
It is crucial to correctly handle unsuccessful transfers in your protocol. The demonstration bug is described
here.
Direct transfer from the protocol means that protocol sends tokens to some external address (user or contract), so, each function containing
transfer() is a natural "way out" for a possible attacker and must be maximally protected.
This includes a very desirable reentrancy protection. Even when using
safeTransfer() correctly and following the Check-Effects-Interactions pattern, in some cases such functions can still be exploited (for example, the ones with "cross-contract" or "read-only" reentrancy). Special attention should be paid to ERC-677 and ERC-777 tokens with
transferAndCall() callbacks, receive hooks, and any other possibilities for calling additional code "inside" the transfer. An example of a bug where
transfer() with reentrancy is used is
here.
Additionally, consider the following potential issues:
- Transferring zero tokens: in some cases, it can lead to problems, which will be discussed in the "approve" section.
- Controllable revert: If an attacker can cause the protocol to always (or controllably) revert, it can lead to problems. A good example, demonstrating both problems is here.