omni / tokenbridge-contracts

Smart contracts for TokenBridge
http://docs.tokenbridge.net
GNU General Public License v3.0
230 stars 228 forks source link

Warning: Etherscan will show as reverted error #81

Closed rstormsf closed 6 years ago

rstormsf commented 6 years ago

Since we added optional call if the recepient is contract etherscan will mark it as an error if contract doesn't have a fallback

function transfer(address _to, uint256 _value) public returns (bool)
    {
        require(superTransfer(_to, _value));
        if (isContract(_to)) {
            contractFallback(_to, _value, new bytes(0));
        }
        return true;
    }

image

You might want to reconsider this.

This functionality is not used anywhere in production yet, but I just wanted to let you know about it.

Everything is still functioning properly, but I don't think it's good idea to have since many people will just freak out by this error while everything is actually fine.

This will happen if a recepient doesn't have onTokenTransfer and then it will call function fallback which if not defined will make a revert()

akolotov commented 6 years ago

Thanks Roman for the report. Could you clarify more the use case? "Some one would like to transfer tokens to the contract which does not support onTokenTransfer()", right?

rstormsf commented 6 years ago

@akolotov correct. Imagine just sending to your Multisig/DEX/any other contract that doesn't have that function and seeing the reverted transaction. Won't be fun

https://etherscan.io/tx/0xb3874442fdfa8b81a7f8b91ab8fa80717dc60e569dba84446125956d67be6855

example

akolotov commented 6 years ago

Hmmm

If an user just does transfer of our token to a contract which has no onTokenTransfer. You described the process right that if onTokenTransfer does not exists in the contract-receiver of token, fallback will throw. But we use call to invoke onTokenTransfer and it means that the executing method is reverted call will get false (https://solidity.readthedocs.io/en/v0.4.25/units-and-global-variables.html#address-related) and will this will not cause revert().

https://github.com/poanetwork/poa-bridge-contracts/blob/4e869c0c06dcff4d1d38ca65e9a62e973f5c3e97/contracts/ERC677BridgeToken.sol#L43-L50

https://github.com/poanetwork/poa-bridge-contracts/blob/4e869c0c06dcff4d1d38ca65e9a62e973f5c3e97/contracts/ERC677BridgeToken.sol#L52-L57

If in your case transfer is reverted, most probably there is another reason for this rather than absence of onTokenTransfer in the contract-receiver.

rstormsf commented 6 years ago

it will. I tried. it will call revert opcode, although the tx status will be success...it will be an internal revert

Please understand the difference between main thread revert where the whole tx fails and where there is internal call where those reverts don't affect the main tx thread. Hence why when there is false return from call in mean inside of that call there was a revert OPCODE I'm not sure how else I can explain it

rstormsf commented 6 years ago

https://kovan.etherscan.io/tx/0x7e0e2852207d611da10ab90b992167c2ab56914af03d8c4e5ddfe7e50bbe7375

see this example.

Successful erc20 transfer but it's annoying for users to see that something went wrong. Check Parity trace, you will see a revert opcode. https://kovan.etherscan.io/vmtrace?txhash=0x7e0e2852207d611da10ab90b992167c2ab56914af03d8c4e5ddfe7e50bbe7375&type=parity

akolotov commented 6 years ago

Ok. I see what you mean. For me, it is not the issue with our contracts but the issue with etherscan.

rstormsf commented 6 years ago

up to you...as long as you are aware of it.

akolotov commented 6 years ago

Closed since there is no issue on the contracts' side.