[Audit Report] [N5] [Suggestion] Token compatibility reminder

[orionstardust]

Description

In the LibERC20 contract, the handleReturn function handles token return values. But when the case is success and the transfered tokens do not have a return value, this will bypass the inspection and revert.

https://github.com/aavegotchi/aavegotchi-realm-diamond/blob/cee38d37307c49dc41cdb737a962d5d313c1cd4f/contracts/libraries/LibERC20.sol#L41

Solution

It is recommended not to use tokens with no return value.

[orionstardust]

@cinnabarhorse @mudgen Their suggestion is for the use case of LibERC20 contract. In realm diamond, LibERC20 is used in craftInstallations() function only. But this is used with Alchemica tokens, and Alchemica token transfer functions have return values. So there's no problem for realm diamond right now. But this library is used in Aavegotchi diamond and GHST staking also. We need to check all use cases of them. And we can add some notes to the function document.

[cinnabarhorse]

Can you give more context to this issue? In which cases will it revert, and which functions are affected.

[mudgen]

But when the case is success and the transfered tokens do not have a return value, this will bypass the inspection and revert.

I don't know why they say this because the case of no return value does not revert according to this code:

https://github.com/aavegotchi/aavegotchi-realm-diamond/blob/cee38d37307c49dc41cdb737a962d5d313c1cd4f/contracts/libraries/LibERC20.sol#L42-L45

[mudgen]

I did a test. I used LibERC20.transfer on an ERC20 token contract that returns no value. There was no revert and it worked fine.

I also looked at how OpenZeppelin implemented SafeERC20.transfer and I found that they handle the optional return value from ERC20.transfer and ERC20.transferFrom the same way that LibERC20 does.

So it looks to me that there is no problem with LibERC20 handling return values or not.