Add validation in Governor on ERC-721 or ERC-1155 received

[clauBv23]

Fixes #4307

Add validation to onERC1155Received, onERC1155BatchReceived, and onERC721Received hooks in Governor to check it is the executor.
Add test to the validations.

Note: This PR should be refactored after merging #4261 and #4284

PR Checklist

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 2d53fa7

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

[Amxx]

If possible, the expectRevert should not be unspecified.

[clauBv23]

@Amxx good catch! But what do you mean whit if possible, is there any specific reason for not specifying it?
In this case for example what is the reason for not doing it?

openzeppelin-contracts/test/governance/extensions/GovernorTimelockCompound.test.js

Lines 88 to 90 in ffceb3c

	it("doesn't accept ether transfers", async function () {
	await expectRevert.unspecified(web3.eth.sendTransaction({ from: owner, to: this.mock.address, value: 1 }));
	});

[Amxx]

@Amxx good catch! But what do you mean whit if possible, is there any specific reason for not specifying it? In this case for example what is the reason for not doing it?

openzeppelin-contracts/test/governance/extensions/GovernorTimelockCompound.test.js

Lines 88 to 90 in ffceb3c

	it("doesn't accept ether transfers", async function () {
	await expectRevert.unspecified(web3.eth.sendTransaction({ from: owner, to: this.mock.address, value: 1 }));
	});

If there is no revert string, or if the revert is a panic, then unspecified might be the way.
Also, maybe some of our current unspecified should be specified.

[clauBv23]

oh perfect makes sense @Amxx thanks!

[ernestognw]

This looks good @clauBv23! Thanks. I'm adding a few suggestions and also requesting review from others.

Thanks!

[Amxx]

LGTM

[Amxx]

@frangio @ernestognw I merged master into this and realised the following:

ERC1155 acceptance hook was written in a way that bubble error string, but not custom error. The result is that the Governor custom error (GovernorDisabledDeposit) is replaced by the default one (ERC1155InvalidReceiver)

Do we want to change the ERC1155 hook to use the syntax from ERC721 (which bubble the custom errors)?

[frangio]

Do we want to change the ERC1155 hook to use the syntax from ERC721 (which bubble the custom errors)?

I'd say yes.