Summary

During our comprehensive contract audit, we have noticed that the contracts employ standard ERC20 calls for token transfer operations. We recommend replacing these with OpenZeppelin's SafeERC20 library calls. This suggestion is rooted in several potential risks that stem from non-standard ERC20 token behaviors. A variety of ERC20 implementations exist that do not comply with the original standard, and one such deviation is not returning a boolean on transfer and transferFrom calls. In case such a token interacts with the contract, the ERC20 call can result in unexpected outcomes.

Vulnerability Details

This code assumes the ERC20 token adheres to the standard perfectly, which is not always the case in reality.

In DSCEngine.sol

There are two cases of this occuring, in functions depositCollateral(), and _redeemCollateral()

depositCollateral()

The purpose of the depositCollateral() function is to allow users to deposit their collateral assets into the DSC system. Below is a snippet of the lines where this occurs.

The depositCollateral() function allows users to deposit their collateral assets into the DSC system. In the process of transferring the collateral tokens from the user's address to the contract's address, it calls the standard ERC20 transferFrom() function. This function typically returns a boolean value indicating whether the transfer was successful. However, in some cases, certain tokens may not return a value (boolean) upon executing the transferFrom() function. This absence of a return value can lead to unexpected behavior. For instance, the contract might interpret the lack of a return value as a false (indicating the operation failed), causing it to revert a transaction that was actually successful. To handle such cases, the depositCollateral() function checks the success of the transferFrom() operation and reverts with an error message (DSCEngine__TransferFailed()) if the transfer fails.

Impact

❌Loss of funds risk

This isn't an exploitable condition, but it will disrupt the normal operation of the DSCEngine contract.
Here's an example of how this could happen:

1. User A calls depositCollateral on the contract with an amount of token X.
   

2. Token X's transferFrom function is poorly implemented and doesn't return a boolean value upon execution. Instead, it modifies the state (transfers the tokens) and then simply ends without return statement. This is a violation of the ERC20 standard but can happen due to a programming error or oversight by the token developer.
   

3. The contract calls token X's transferFrom function. Since there is no return value, Solidity automatically assigns false to the success variable.
   

4. The contract checks the success variable, finds it to be false, and reverts the transaction, believing the transfer failed.
   

5. However, in reality, the token transfer was successful. The user's balance of token X decreased, but the contract rejected the transaction, leading to an inconsistency between the state of the contract and the actual token balance.

Tools Used

VSCode, Foundry

Recommendations

Using OpenZeppelin's SafeERC20 library would make the function safer as it handles these edge cases. It treats tokens that don't return a value as if they returned true, and it also converts false returns into reverts, which allows the contract to fail early and visibly.

Example:

The code is utilizing the SafeERC20 library, which is a secure wrapper for the IERC20 interface. In this context, the safeTransferFrom function is being used to transfer amountCollateral tokens from the sender's address (msg.sender) to the contract's address (address(this)). The SafeERC20 library ensures that the transfer is handled safely by taking care of potential edge cases, such as tokens that do not return a value upon transfer or tokens that may cause unexpected behavior. By using SafeERC20, the contract mitigates the risk of potential issues and ensures that token transfers are executed securely and reliably."