👓Examples

Book an audit with Zokyo

Understanding the Examples: In the "Examples" section, we delve into practical instances of vulnerabilities associated with ERC20 transfer functions and how they were mitigated using OpenZeppelin's SafeERC20 safe transfers. These examples are taken from real-world audit reports, bug bounty programs, and in-depth code analyses. By studying these real-life cases, you'll gain a deeper and more tangible understanding of the intricacies of token transfers and their potential vulnerabilities. This knowledge will enhance your skills in identifying potential issues, applying safe transfer techniques, and effectively auditing smart contract code.

[LOW] Unsafe use of transfer()/transferFrom() with IERC20

Axelar Code4rena Contest bug report with payout

Code4renaCode4rena

ERC20 is a standard interface for tokens within smart contracts. This standard provides a set of functions which any token implementing ERC20 must adhere to. The functions include transfer(), transferFrom(), and others. In accordance with the ERC20 standard, the transfer() and transferFrom() functions are expected to return a boolean value indicating the success or failure of the operation.

However, some tokens do not strictly adhere to this standard, despite being generally accepted as ERC20 tokens. A notable example of such a token is Tether (USDT). The transfer() and transferFrom() functions in Tether's smart contract do not return a boolean value as expected per the ERC20 standard, instead, they return nothing.

This deviation from the standard can lead to unexpected issues. When such tokens are cast, or treated as, IERC20 tokens (which means the interacting code assumes the token complies fully with the ERC20 standard), problems arise due to the mismatch between the expected function signatures (which include the return type) and the actual implemented functions. Specifically, calls to the transfer() or transferFrom() functions would fail, and the whole transaction would be reverted, because the functions don't return anything, whereas the code expects a boolean value.

As a mitigation to this issue, developers can use the safeTransfer() and safeTransferFrom() functions provided by the OpenZeppelin library's SafeERC20 module. These functions are designed to interact with ERC20 tokens in a way that handles these deviations from the standard. They allow for successful interactions even with tokens that do not strictly follow the ERC20 standard, such as Tether, by not relying on the return value. This way, even if the transfer() or transferFrom() functions don't return a value, the safeTransfer() and safeTransferFrom() functions will still work as expected, preventing unexpected transaction reverts.

[LOW] Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

Another Code4rena bug report with payout

Quote: "It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract."

High Vulnerability findings

Typically, when a smart contract interacts with an ERC20 token, it often performs a transfer() or transferFrom() operation to move tokens. As mentioned earlier, these operations are expected to return a boolean indicating whether the operation was successful or not. If a token, like Tether (USDT), does not adhere strictly to the ERC20 standard and does not return any value from these operations, smart contracts expecting a return value will see these operations as failed. This will cause the entire transaction to revert.

Now, imagine a scenario where a smart contract is designed to accept any ERC20 token, perform certain operations (like staking, pooling, etc.), and then allow users to withdraw their tokens when they want to. If a user deposits a non-standard token like USDT into the contract and the contract later tries to transfer those tokens (using transfer() or transferFrom()), it would trigger a transaction revert due to the lack of a return value from the transfer operations.

The critical point here is that if the contract doesn't have a special mechanism in place to handle such non-standard tokens, the user's tokens become effectively trapped within the contract. The contract cannot perform any transfer operations with the tokens, and the user cannot withdraw their tokens, resulting in a loss. In a worst-case scenario, if the contract manages a large pool of such tokens, the financial implications could be significant.

That's why using functions like safeTransfer() or safeTransferFrom() from the OpenZeppelin SafeERC20 library is essential. They can handle tokens even if they don't strictly follow the ERC20 standard, reducing the risk of tokens becoming trapped within a contract.

Examples:

[HIGH] TokenHandler.safeTransferOut does not work on non-standard compliant tokens like USDT

Code4rea bug report with payout

In the given scenario, the TokenHandler.safeTransferOut function is designed with the intention of safely managing token transfers, even for non-standard ERC20 tokens that do not return a boolean value when the transfer() function is called.

However, the logic it employs is flawed. The function first makes a call to the standard IERC20.transfer method, and then it executes a checkReturnCode function designed to handle non-standard tokens. The issue lies in the sequence of these operations. The IERC20.transfer call is expected to return a boolean indicating the success or failure of the transfer operation. If it doesn't receive a boolean (which is the case with non-standard tokens like USDT), the EVM considers this as a failed operation and the transaction reverts immediately. As a result, the subsequent checkReturnCode function is never even executed.

In effect, the function's current design prevents it from fulfilling its intended role. When users attempt to deposit non-standard tokens, the function reverts, effectively making deposits for these tokens impossible. This issue is significant because these non-standard tokens, including USDT, are widely used as valid underlying assets for various cTokens such as cUSDT.

The recommended solution to this problem is to use the safeApprove function from OpenZeppelin's SafeERC20 library, as it is specifically designed to handle ERC20 tokens, whether standard-compliant or not. The safeApprove function includes mechanisms to check the return value from approval operations and can correctly handle tokens that don't return a value, ensuring that the transaction does not revert unexpectedly and the tokens can be successfully managed.

[HIGH]TokenHandler.safeTransferIn does not work on non-standard compliant tokens like USDT

Code4rea bug report with payout

Details: The TokenHandler.safeTransferIn function uses the standard IERC20 function for the transfer call and proceeds with a checkReturnCode function to handle non-standard compliant tokens that don't return a return value. However, this does not work as calling token.transferFrom(account, amount) already reverts if the token does not return a return value, as token's IERC20.transferFrom is defined to always return a boolean.

Impact: When using any non-standard compliant token like USDT, the function will revert. Withdrawals for these tokens are broken, which is bad as USDT is a valid underlying for the cUSDT cToken.

Recommended Mitigation Steps: We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.