First Flight #16: Mafia Takedown

Summary

Not all ERC20 implementations revert() when there's a failure in transfer() or transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually transfer anything.

Vulnerability Details

Some tokens do not fully comply with the ERC20 standard but are still widely accepted. For example, Tether (USDT) has transfer() and transferFrom() functions that do not return booleans as required, causing calls to revert when cast to IERC20. To handle these tokens correctly

There are 2 instances of this issue:

Impact

If the usdc.transferFrom call fails, the contract can still call deposit(to, amount) and crimeMoney.mint(to, amount) leading to unauthorized minting of crimeMoney tokens.

Tools Used

Manual Review

Recommendations

The transfer of USDC should be checked to ensure it is successful before proceeding with the deposit and mint operations.

In the depositUSDC() function

And in the withdrawUSDC() function

Or use OpenZeppelin's SafeERC20 library with safeTransfer() and safeTransferFrom(),