No ERC20 safe* versions called & no return values checked

[code423n4]

Handle

cmichel

Vulnerability details

Some tokens (like USDT) don't correctly implement the EIP20 standard and their transfer/transferFrom function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.

Non-safe transfers are used in:

• Zapper.recoverFunds
• IdentityFactory.withdraw

These occurrences also do not check the success return boolean of the transfer.

Impact

Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

[Ivshti]

partial duplicate of #9

Here's why this won't be fixed:

• in zapper, it doesn't matter, since this method is for recovering funds - it doesn't need to revert in case the recovery was not successful
• in the IdentityFactory, the same logic applies; it's only used in extraordinary circumstances such as the factory receiving some funds by accident - and it doesn't need to revert on failure

Most importantly, it is not true that those will revert in case the token does not return a boolean, since we do not check the revert value. Whatever is in the stack will be cast as a boolean, but since we do not require() it, USDT/BNB/etc. won't have a problem.

[GalloDaSballo]

Will need to dig deeper to fully understand this one

That said:

If a transfer fails and returns false, and you don't check for it (i.e no require) the transaction won't revert, it's simply that the tokens may have not been transferred

As for the context through which the findings were sent, that's why I'll need more time to properly judge the severity

In terms of mitigation: Since the functions are meant to withdraw stuck funds, in exceptional scenarios, there's no risk and probably no need for any change for the sponsor

[GalloDaSballo]

After checking the source code, I'm reducing the severity to non-critical, the only "risk" is running a transaction that won't move funds

The funds will not be stuck because of this implementation The funds won't be permanently lost because of this implementation There is no risk for funds

It simply doesn't check the return value, and will not revert on failure, just spend the extra gas on that occasion

Keeping this as the main issue and setting rest to duplicates