Overriding ERC20's transfer function

[chriscrutt]

Aloha. I love that ERC1363 allows you/people to interact with contracts in a more robust way- such as with onTransferReceived where you can interact with the tokens when receiving them as opposed to having to approve them and then transfer them out of the person's account in order to interact with the tokens.

I had an idea, however, for a staking contract. So whenever ERC1363 tokens are sent to the smart contract it stakes them for the sender. Now this is super easy when transferAndCall is used by the sender as we can react to the tokens coming in with onTransferReceived, but what if just transfer is called by somebody that doesn't know better?

I suggest an override for the transfer function that checks to see if the receiver address is a smart contract or not, and then runs _checkOnTransferReceived accordingly. We could do this by overriding _afterTokenTransfer.

function _afterTokenTransfer(
    address from,
    address to,
    uint256 amount
) internal virtual override {    
    if (to.isContract()) {
        _checkOnTransferReceived(from, to, amount, "");
    }
    super._afterTokenTransfer(from, to, amount);
}

I am not sure if this would mess up minting functions and such. It might also be okay to override the _transfer function directly. The code above would potentially prompt _checkOnTransferReceived to be changed as it would check to see if the recipient was a contract address a second time with

if (!recipient.isContract()) {
    revert("ERC1363: transfer to non contract address");
}

edit: I am also realizing that changing _transfer/_afterTokenTransfer might be an issue for existing contracts and liquidity pools... unless another wrapper contract is deployed that converts it to just an ERC20

[vittominacori]

Thanks for pointing out this.

The purpose of the EIP is to force people using the callback behavior when they know what they are doing so we fixed to force transferAndCall (or approveAndCall) reverting if the receiver contract is not an ERC1363Receiver.

If you force the standard ERC20::transfer to work as the ERC1363::transferAndCall you may break those tokens to be sent to a non ERC1363 receiver, because transferring to a general contract will fail.

You could check if the receiver accept the ERC1363 interface using ERC165 before calling _checkOnTransferReceived but it is not the purpose of this repository. You can extend and override these methods in a derived contract.

[chriscrutt]

So tokens can still be lost by someone who doesn't know better if they just transfer tokens directly to a smart contract that doesn't have a way to handle them (if the smart contract doesn't have a refund/withdraw function)? And, to sorta restate what you were saying, the EIP is more to allow those who know what they're doing to utilize/force a callback behavior?

[vittominacori]

If transfer behaves like the transferAndCall it must revert to a contract that doesn't have the ERC1363 Receiver. So basically sending tokens to a normal contract will always revert (as you can see here).

Yes the EIP purpose is to give a way to make callback after a transfer but the receiver contract MUST implement the method in order to use the received tokens.

[chriscrutt]

yeah, what I'm saying is _checkOnTransferReceived is only implemented on transferAndCall, not transfer. So someone could still call the transfer function since ERC1363 inherits ERC20 and send it to a smart contract and lose their funds in that way.

[vittominacori]

Yes, the EIP doesn't prevent wrong transfer.