Governor, TimelockController, and similar, should be ERC721Holder

[frangio]

Contracts that can send arbitrary transactions through some protocol, like Governor and TimelockController, should contain the required hook to receive ERC721 tokens via safeTransfer (which we implement in ERC721Holder).

I think there is a similar hook for ERC1155.

[Amxx]

I 100% agree this should be a thing. These contracts should be ERC721Holder and ERC1155Holder

There is an issue however: ERC721HolderUpgradeable contains a gap. If we add ERC721Holder to Governor, the transpilled version will not be storage compatible. This is a mess that won't be fixed until we radically transition to "diamond storage"

[frangio]

Good point. We will have to include the logic without using ERC721Holder.

[ashwinYardi]

Hey guys! @frangio @Amxx Just to confirm, this basically means that , Governor and timelock contracts should have onERC1155Received and onERC721Received hooks, correct?

[Amxx]

@ashwinYardi Yes, and also onERC1155BatchReceived

[ashwinYardi]

Cool. Also I think metatx/MinimalForwarder.sol also needs these hooks. What say? @Amxx @frangio

[Amxx]

No, the MinimalForwarder is completely unrelated. It's only supposed to forward call, not ever receive any assets. If it did receive assets, anyone could likely claim them (unless they are forwarder aware, but that is unlikely).

We should NOT put these functions in the MinimalForwarder, as it might prevent someone from inadvertently sending assets to it.

[ashwinYardi]

Ahh, all right! Will revert the changes I did in MinimalForwarder 😅

[ashwinYardi]

#3230 This is ready. @frangio @Amxx