Summary

A vulnerability was found that relates to token compliance with the ERC20 standard. Some tokens like USDT do not fully comply with the ERC20 standard and return nothing instead of a boolean value in tranferFrom or transfer. This would lead to transactions getting reverted since success would still be false even if the transaction succeeds. Note that this issue is different from the already known finding as the main issue here has to do with the if statement after the transfer and transferFrom methods.

Vulnerability Details

In DSCEngine.sol, the functions depositCollateral, _redeemCollateral and _burnDsc all use the transfer or transferFrom method in the implementation, and the return value is stored in a variable named success. However, due to some ERC20 tokens non-compliance, if a token like USDT is used as collateral, the return value would always be false. As a result, the if(!success) would always be triggered, and the transaction reverted.

Impact

The impact is considerable as it can prevent the execution of transactions, even if they are valid. Since the contract is designed in such a way that multiple tokens can be used as stated in the description:

This project is meant to be a stablecoin where users can deposit WETH and WBTC in exchange for a token that will be pegged to the USD. The system is meant to be such that someone could fork this codebase, swap out WETH & WBTC for any basket of assets they like, and the code would work the same.

This issue can lead to loss of trust in the contract, and prevent users from using it effectively.

Tools Used

Manual review

Recommendations

A warning can be placed on the dangers on using the contract with Non-standard ERC20 tokens like USDT OR safeTransfer and safeTransferFrom can be used instead.