Add ERC1363 implementation

[vittominacori]

This PR adds the ERC1363 implementation following the v5 guidelines.

It uses inheritdoc whenever is possible and custom errors following the ERC-6093 rationale.

Would like to replace #3525 and #3017 as they are outdated.

Fixes: #3736
Fixes LIB-1181

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 2c85474

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[zicoz18]

NOT DIRECTLY RELATED TO THE PR:

I have been looking at some standards such as ERC777 and ERC1363. ERC777 used to be included in OpenZeppelin contract and it got removed. I was wondering why such standards that extend ERC20 to be able to add a call to transfers or approvals is not highly adopted. Since they are backwards compatible, why not use them on new tokens? I am considering to use them but want to learn the reason why others are not using it. Maybe related to complexity, extra work for integrations, security reasons (possible reentrancy in on tokensReceived for ERC777 and onTransferReceived for ERC1363) or maybe UX reasons? Is there any sources that discuss the trade-offs?

[Amxx]

I was wondering why such standards that extend ERC20 to be able to add a call to transfers or approvals is not highly adopted

For ERC777:
Partly because its a pain to work with (smart contracts can't receive them without first registring in the 1822 registry) and because they are notoriously dangerous. ERC777 opens a lot of reentrancy paths that are dangerous if not properly considered.

[Amxx]

Is there any sources that discuss the trade-offs?

This one obviously comes to mind: #2620
There are probably many others

[zicoz18]

This one obviously comes to mind: #2620 There are probably many others

Thanks a lot for the answers 🙏

[ernestognw]

Haven't reviewed the tests, I'll push some updates

[ernestognw]

Still pending to review ERC1363 tests.

[ernestognw]

LGTM 🚀

[vittominacori]

I missed why it is saying that target MUST implement the interface but only that target SHOULD return interface id.

It reverts if target returns something different than the interface id.

(also transferFromAndCall and approveAndCall)

openzeppelin-contracts/contracts/token/ERC20/extensions/ERC1363.sol

Lines 57 to 66 in e86bb45

	* Requirements:
	*
	* - The target has code (i.e. is a contract).
	* - The target `to` must implement the {IERC1363Receiver} interface.
	* - The target should return the {IERC1363Receiver} interface id.
	* - The internal {transfer} must succeed (returned `true`).
	*/
	function transferAndCall(address to, uint256 value) public returns (bool) {
	return transferAndCall(to, value, "");
	}

[vittominacori]

@Amxx @ernestognw I'm not sure about checking ERC20 methods return values.

The ERC1363 does not explicitly say that the ERC20 methods MUST succeed.

It says:

Defines a token interface for ERC-20 tokens that supports executing recipient code after transfer or transferFrom, or spender code after approve.

Below it says:

#notice Transfer tokens from msg.sender to another address and then call onTransferReceived on receiver

Maybe logically ok to force checking that the ERC20 methods returned true as it says "Transfer tokens..." but IMO it is not required.

---

Also regarding ERC1363 returning false, the spec says that methods:

#return true unless throwing

So it is unnecessary to test ERC1363 against returning false as it is out of the standard.

[ernestognw]

@Amxx @ernestognw I'm not sure about checking ERC20 methods return values.
The ERC1363 does not explicitly say that the ERC20 methods MUST succeed.

Yeah but also it doesn't mention they have to be treated as a noop. We concluded that it's more dangerous to allow a noop because an attacker can force a transfer failure and still call the target. This is misleading and may compromise the assumption that the caller is trusted (or always is called after a successful transfer).

I also believed it shouldn't be required, but we strive for the most secure implementation out of the box, and I do think this is it.

Also regarding ERC1363 returning false, the spec says that methods:

#return true unless throwing

So it is unnecessary to test ERC1363 against returning false as it is out of the standard.

Yeah. However, that's not the case for approveAndCall.
I agree we shouldn't guarantee compatibility with non-standard ERC1363, but this is also a design decision based on the likelihood of making a mistake (by returning false) and the inconsistency between the return param definitions.

I think I'd be open to changing this. It's unclear to me if there's a potential issue arising, but I think reverting is the safest. Compliant tokens will work perfectly using the SafeERC20 functions