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

.changeset/mighty-donuts-smile.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+Add validation in Governor ERC1155 and ERC721 receiver hooks to ensure Governor is the executor

contracts/governance/Governor.sol

@@ -604,13 +604,16 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
 
     /**
      * @dev See {IERC721Receiver-onERC721Received}.
+     * Receiving tokens is disabled if the governance executor is other than the governor itself (eg. when using with a timelock).
      */
     function onERC721Received(address, address, uint256, bytes memory) public virtual override returns (bytes4) {
+        require(_executor() == address(this), "Governor: must send to executor");
         return this.onERC721Received.selector;
     }
 
     /**
      * @dev See {IERC1155Receiver-onERC1155Received}.
+     * Receiving tokens is disabled if the governance executor is other than the governor itself (eg. when using with a timelock).
      */
     function onERC1155Received(
         address,
@@ -619,11 +622,13 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
         uint256,
         bytes memory
     ) public virtual override returns (bytes4) {
+        require(_executor() == address(this), "Governor: must send to executor");
         return this.onERC1155Received.selector;
     }
 
     /**
      * @dev See {IERC1155Receiver-onERC1155BatchReceived}.
+     * Receiving tokens is disabled if the governance executor is other than the governor itself (eg. when using with a timelock).
      */
     function onERC1155BatchReceived(
         address,
@@ -632,6 +637,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
         uint256[] memory,
         bytes memory
     ) public virtual override returns (bytes4) {
+        require(_executor() == address(this), "Governor: must send to executor");
         return this.onERC1155BatchReceived.selector;
     }
 }

test/governance/extensions/GovernorTimelockCompound.test.js

@@ -10,6 +10,8 @@ const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsI
 const Timelock = artifacts.require('CompTimelock');
 const Governor = artifacts.require('$GovernorTimelockCompoundMock');
 const CallReceiver = artifacts.require('CallReceiverMock');
+const ERC721 = artifacts.require('$ERC721');
+const ERC1155 = artifacts.require('$ERC1155');
 
 function makeContractAddress(creator, nonce) {
   return web3.utils.toChecksumAddress(
@@ -202,6 +204,67 @@ contract('GovernorTimelockCompound', function (accounts) {
             await expectRevert(this.helper.execute(), 'Governor: proposal not successful');
           });
         });
+
+        describe('on safe receive', function () {
+          describe('ERC721', function () {
+            const name = 'Non Fungible Token';
+            const symbol = 'NFT';
+            const tokenId = web3.utils.toBN(1);
+
+            beforeEach(async function () {
+              this.token = await ERC721.new(name, symbol);
+              await this.token.$_mint(owner, tokenId);
+            });
+
+            it("can't receive an ERC721 safeTransfer", async function () {
+              await expectRevert(
+                this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }),
+                'Governor: must send to executor',
+              );
+            });
+          });
+
+          describe('ERC1155', function () {
+            const uri = 'https://token-cdn-domain/{id}.json';
+            const tokenIds = {
+              1: web3.utils.toBN(1000),
+              2: web3.utils.toBN(2000),
+              3: web3.utils.toBN(3000),
+            };
+
+            beforeEach(async function () {
+              this.token = await ERC1155.new(uri);
+              await this.token.$_mintBatch(owner, Object.keys(tokenIds), Object.values(tokenIds), '0x');
+            });
+
+            it("can't receive ERC1155 safeTransfer", async function () {
+              await expectRevert(
+                this.token.safeTransferFrom(
+                  owner,
+                  this.mock.address,
+                  ...Object.entries(tokenIds)[0], // id + amount
+                  '0x',
+                  { from: owner },
+                ),
+                'Governor: must send to executor',
+              );
+            });
+
+            it("can't receive ERC1155 safeBatchTransfer", async function () {
+              await expectRevert(
+                this.token.safeBatchTransferFrom(
+                  owner,
+                  this.mock.address,
+                  Object.keys(tokenIds),
+                  Object.values(tokenIds),
+                  '0x',
+                  { from: owner },
+                ),
+                'Governor: must send to executor',
+              );
+            });
+          });
+        });
       });
 
       describe('cancel', function () {

test/governance/extensions/GovernorTimelockControl.test.js

@@ -9,6 +9,8 @@ const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsI
 const Timelock = artifacts.require('TimelockController');
 const Governor = artifacts.require('$GovernorTimelockControlMock');
 const CallReceiver = artifacts.require('CallReceiverMock');
+const ERC721 = artifacts.require('$ERC721');
+const ERC1155 = artifacts.require('$ERC1155');
 
 const TOKENS = [
   { Token: artifacts.require('$ERC20Votes'), mode: 'blocknumber' },
@@ -439,6 +441,67 @@ contract('GovernorTimelockControl', function (accounts) {
             // then coverage reports.
           });
         });
+
+        describe('on safe receive', function () {
+          describe('ERC721', function () {
+            const name = 'Non Fungible Token';
+            const symbol = 'NFT';
+            const tokenId = web3.utils.toBN(1);
+
+            beforeEach(async function () {
+              this.token = await ERC721.new(name, symbol);
+              await this.token.$_mint(owner, tokenId);
+            });
+
+            it("can't receive an ERC721 safeTransfer", async function () {
+              await expectRevert(
+                this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }),
+                'Governor: must send to executor',
+              );
+            });
+          });
+
+          describe('ERC1155', function () {
+            const uri = 'https://token-cdn-domain/{id}.json';
+            const tokenIds = {
+              1: web3.utils.toBN(1000),
+              2: web3.utils.toBN(2000),
+              3: web3.utils.toBN(3000),
+            };
+
+            beforeEach(async function () {
+              this.token = await ERC1155.new(uri);
+              await this.token.$_mintBatch(owner, Object.keys(tokenIds), Object.values(tokenIds), '0x');
+            });
+
+            it("can't receive ERC1155 safeTransfer", async function () {
+              await expectRevert(
+                this.token.safeTransferFrom(
+                  owner,
+                  this.mock.address,
+                  ...Object.entries(tokenIds)[0], // id + amount
+                  '0x',
+                  { from: owner },
+                ),
+                'Governor: must send to executor',
+              );
+            });
+
+            it("can't receive ERC1155 safeBatchTransfer", async function () {
+              await expectRevert(
+                this.token.safeBatchTransferFrom(
+                  owner,
+                  this.mock.address,
+                  Object.keys(tokenIds),
+                  Object.values(tokenIds),
+                  '0x',
+                  { from: owner },
+                ),
+                'Governor: must send to executor',
+              );
+            });
+          });
+        });
       });
     });
   }