15,000 USDC
View results
Submission Details
Severity: medium
Valid

Replace `transferFrom()` with `safeTransferFrom()`

Summary

The DSCEngine contract utilizes the transferFrom() function from the OpenZeppelin IERC20 interface to transfer tokens. However, this could potentially lead to undesired consequences. While the contract checks for the success of the transferFrom() operation, this check is not entirely reliable as it assumes the token contract adheres to the ERC20 standard completely. Not all token contracts return a boolean value upon the execution of transferFrom(). Some contracts might revert on failure, while others might incorrectly always return true, even if the transfer fails.

Vulnerability Details

The vulnerability lies in the use of the transferFrom() function without validating the actual result of the transfer. If a token contract doesn't adhere to the standard ERC20 implementation and always returns true regardless of the transfer's success, it might cause issues. The following code snippet represents the relevant section of the DSCEngine contract where transferFrom() is used:

function depositCollateral(address tokenCollateralAddress, uint256 amountCollateral)
public
moreThanZero(amountCollateral)
isAllowedToken(tokenCollateralAddress)
nonReentrant
{
s_collateralDeposited[msg.sender][tokenCollateralAddress] += amountCollateral;
emit CollateralDeposited(msg.sender, tokenCollateralAddress, amountCollateral);
bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
if (!success) {
revert DSCEngine__TransferFailed();
}
}

Impact

If a token contract always returns true, regardless of whether the transferFrom() function succeeds or fails, the DSCEngine contract will assume that the transfer was successful. This discrepancy between the actual token balance and the internal accounting of the DSCEngine contract could lead to potential fund loss or unexpected behavior.

Tools Used

Manual review

Recommendations

To mitigate the vulnerability mentioned above, it is recommended to use the safeTransferFrom() function from the OpenZeppelin library instead of the transferFrom(). The safeTransferFrom() function checks the return value and reverts if the return value is not true, providing an extra layer of safety.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.