Replace safety transfer acceptance check calls with Address.functionCall

[qiweiii]

Fixes #4264

📝 Details Both ERC1155 and ERC721 make transfer acceptance checks and they catch errors to bubble them up. However, this behavior is already implemented in Address.sol.

For consistency, it's required to use Address.functionCall(address target, bytes memory data) instead

Replaced direct onERC721Received and onERC1155BatchReceived calls with Address.functionCall.

No test changes since does not modify existing behaviour.

PR Checklist

• [x] Tests
• [x] Documentation

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: e8bb603bee8ee666cc8afc0c22dc54cf88bbd4aa

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

[ernestognw]

As a side note, surprisingly the gas costs seem to have increased consistently.

https://github.com/OpenZeppelin/openzeppelin-contracts/actions/runs/5055405317

I'd expect otherwise, but I'd like to double check, if that's the case, this may need fuether discussion

[Amxx]

Note that we are doing some changes to the way Address.functionCall handles the default revert. We should wait for that refactor to be complete and then further analyse the impact of that change.

Also, we'll want to use abi.encodeCall

[qiweiii]

@ernestognw I found a possible answer for increase in gas:

The gas cost comes from the state lookup. To load the code that should be run for a call or delegatecall it is required to perform a lookup in the state tree. If these costs are removed it is easy to perform a denial of service attack on Ethereum.

@Amxx sure, let's wait for the refactor first.

[ernestognw]

Hey @qiweiii, sorry for the late reply, I think the repo is now in good shape for adding this.

The conflicts are because of changes introduced to Address after #4261 and #4399. Also thanks for updating to abi.encodeCall

I'm requesting a review from the team. Please consider this is a 5.0 stretch goal so we might put some priority on other items first.

[frangio]

Note that we are doing some changes to the way Address.functionCall handles the default revert. We should wait for that refactor to be complete and then further analyse the impact of that change.

@Amxx What were you referring to here?

[Amxx]

Note that we are doing some changes to the way Address.functionCall handles the default revert. We should wait for that refactor to be complete and then further analyse the impact of that change.

@Amxx What were you referring to here?

This PR uses Address.functionCall(address, bytes, string). That is something we recently remove