Governor should revert on ERC1155 and ERC721 receive hooks when it's not executor

[ernestognw]

Description

Currently, the Governor implementation uses onERC1155Received, onERC1155BatchReceived, and onERC721Received to acknowledge token receives. However, if the Governor is not the executor itself, it can't operate on those tokens except using the relay function.

We consider that all of these receive hooks can also validate that the Governor is also the executor, as in the receive() function:

/**
 * @dev Function to receive ETH that will be handled by the governor (disabled if executor is a third party contract)
 */
receive() external payable virtual {
    if (_executor() != address(this)) {
        revert GovernorDepositDisabled(address(this));
    }
}

[AnmolSirola]

@ernestognw As it's marked as a good first issue I would like to work on it , could you please provide guidance on how to approach and resolve this issue?
Thank you!

[ernestognw]

Hey @AnmolSirola, thanks for your interest!

Sure, the issue is result from this discussion, we agreed that Governor has no reason to receive tokens if it's no the executor so we're thinking of a check like:

require(address(this) == executor())

We would require also a test for each error to accept the PR, you can instantiate an ERC721 and ERC1155 and use hardhat exposed (already installed in this repo) to call $_safeMint to the governor and trigger the expected error.

Note that the target branch would be master but we're aiming to merge #4261 quite soon.

[clauBv23]

Hi there, I started working on this task before seeing the last comments and opened PR #4314 .

But, after reading the mentioned discussion realized there is an issue and corresponding PR #4284 that proposes to use ERC721Holder & ERC1155Holder instead of the hooks currently defined in Governor, is the expected solution to override ERC721Holder & ERC1155Holder hooks adding the required validation?

Anyway, after merging #4261 and #4284 the PR should be refactored, so not sure if it should be moved to draft