Add ERC721 and ERC1155 receiver support in Governor, Timelock (#3230)

* add ERC721 and ERC1155 receiver support in Governor, Timelock and MinimalForwarder (#3174) * revert the nft receiver hooks from MinimalForwarder and linting updates * add ERC165 support & simplify test * add changelog entry Co-authored-by: Hadrien Croubois <[email protected]>
OpenZeppelin · Mar 24, 2022 · 76fca3a · 76fca3a
1 parent 74c9130
commit 76fca3a
Show file tree

Hide file tree

Showing 5 changed files with 295 additions and 87 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,8 @@
  * `TimelockController`: Add a separate canceller role for the ability to cancel. ([#3165](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3165))
  * `draft-ERC20Permit`: replace `immutable` with `constant` for `_PERMIT_TYPEHASH` since the `keccak256` of string literals is treated specially and the hash is evaluated at compile time. ([#3196](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3196))
  * `ERC20Wrapper`: the `decimals()` function now tries to fetch the value from the underlying token instance. If that calls revert, then the default value is used. ([#3259](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3259))
+ * `Governor`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by governors. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230))
+ * `TimelockController`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by timelocks. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230))
 
 ### Breaking changes
 

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
@@ -3,6 +3,8 @@
 
 pragma solidity ^0.8.0;
 
+import "../token/ERC721/IERC721Receiver.sol";
+import "../token/ERC1155/IERC1155Receiver.sol";
 import "../utils/cryptography/ECDSA.sol";
 import "../utils/cryptography/draft-EIP712.sol";
 import "../utils/introspection/ERC165.sol";
@@ -24,7 +26,7 @@ import "./IGovernor.sol";
  *
  * _Available since v4.3._
  */
-abstract contract Governor is Context, ERC165, EIP712, IGovernor {
+abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receiver, IERC1155Receiver {
     using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque;
     using SafeCast for uint256;
     using Timers for Timers.BlockNumber;
@@ -97,6 +99,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
                 this.castVoteWithReasonAndParamsBySig.selector ^
                 this.getVotesWithParams.selector) ||
             interfaceId == type(IGovernor).interfaceId ||
+            interfaceId == type(IERC1155Receiver).interfaceId ||
             super.supportsInterface(interfaceId);
     }
 
@@ -552,4 +555,42 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     function _executor() internal view virtual returns (address) {
         return address(this);
     }
+
+    /**
+     * @dev See {IERC721Receiver-onERC721Received}.
+     */
+    function onERC721Received(
+        address,
+        address,
+        uint256,
+        bytes memory
+    ) public virtual override returns (bytes4) {
+        return this.onERC721Received.selector;
+    }
+
+    /**
+     * @dev See {IERC1155Receiver-onERC1155Received}.
+     */
+    function onERC1155Received(
+        address,
+        address,
+        uint256,
+        uint256,
+        bytes memory
+    ) public virtual override returns (bytes4) {
+        return this.onERC1155Received.selector;
+    }
+
+    /**
+     * @dev See {IERC1155Receiver-onERC1155BatchReceived}.
+     */
+    function onERC1155BatchReceived(
+        address,
+        address,
+        uint256[] memory,
+        uint256[] memory,
+        bytes memory
+    ) public virtual override returns (bytes4) {
+        return this.onERC1155BatchReceived.selector;
+    }
 }
diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol
@@ -4,6 +4,8 @@
 pragma solidity ^0.8.0;
 
 import "../access/AccessControl.sol";
+import "../token/ERC721/IERC721Receiver.sol";
+import "../token/ERC1155/IERC1155Receiver.sol";
 
 /**
  * @dev Contract module which acts as a timelocked controller. When set as the
@@ -20,7 +22,7 @@ import "../access/AccessControl.sol";
  *
  * _Available since v3.3._
  */
-contract TimelockController is AccessControl {
+contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver {
     bytes32 public constant TIMELOCK_ADMIN_ROLE = keccak256("TIMELOCK_ADMIN_ROLE");
     bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE");
     bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE");
@@ -117,6 +119,13 @@ contract TimelockController is AccessControl {
      */
     receive() external payable {}
 
+    /**
+     * @dev See {IERC165-supportsInterface}.
+     */
+    function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, AccessControl) returns (bool) {
+        return interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId);
+    }
+
     /**
      * @dev Returns whether an id correspond to a registered operation. This
      * includes both Pending, Ready and Done operations.
@@ -365,4 +374,42 @@ contract TimelockController is AccessControl {
         emit MinDelayChange(_minDelay, newDelay);
         _minDelay = newDelay;
     }
+
+    /**
+     * @dev See {IERC721Receiver-onERC721Received}.
+     */
+    function onERC721Received(
+        address,
+        address,
+        uint256,
+        bytes memory
+    ) public virtual override returns (bytes4) {
+        return this.onERC721Received.selector;
+    }
+
+    /**
+     * @dev See {IERC1155Receiver-onERC1155Received}.
+     */
+    function onERC1155Received(
+        address,
+        address,
+        uint256,
+        uint256,
+        bytes memory
+    ) public virtual override returns (bytes4) {
+        return this.onERC1155Received.selector;
+    }
+
+    /**
+     * @dev See {IERC1155Receiver-onERC1155BatchReceived}.
+     */
+    function onERC1155BatchReceived(
+        address,
+        address,
+        uint256[] memory,
+        uint256[] memory,
+        bytes memory
+    ) public virtual override returns (bytes4) {
+        return this.onERC1155BatchReceived.selector;
+    }
 }
diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js
@@ -14,6 +14,8 @@ const {
 const Token = artifacts.require('ERC20VotesMock');
 const Governor = artifacts.require('GovernorMock');
 const CallReceiver = artifacts.require('CallReceiverMock');
+const ERC721Mock = artifacts.require('ERC721Mock');
+const ERC1155Mock = artifacts.require('ERC1155Mock');
 
 contract('Governor', function (accounts) {
   const [ owner, proposer, voter1, voter2, voter3, voter4 ] = accounts;
@@ -55,6 +57,7 @@ contract('Governor', function (accounts) {
 
   shouldSupportInterfaces([
     'ERC165',
+    'ERC1155Receiver',
     'Governor',
     'GovernorWithParams',
   ]);
@@ -574,4 +577,56 @@ contract('Governor', function (accounts) {
       expect(await this.mock.proposalThreshold()).to.be.bignumber.equal('1000000000000000000');
     });
   });
+
+  describe('safe receive', function () {
+    describe('ERC721', function () {
+      const name = 'Non Fungible Token';
+      const symbol = 'NFT';
+      const tokenId = new BN(1);
+
+      beforeEach(async function () {
+        this.token = await ERC721Mock.new(name, symbol);
+        await this.token.mint(owner, tokenId);
+      });
+
+      it('can receive an ERC721 safeTransfer', async function () {
+        await this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner });
+      });
+    });
+
+    describe('ERC1155', function () {
+      const uri = 'https://token-cdn-domain/{id}.json';
+      const tokenIds = {
+        1: new BN(1000),
+        2: new BN(2000),
+        3: new BN(3000),
+      };
+
+      beforeEach(async function () {
+        this.token = await ERC1155Mock.new(uri);
+        await this.token.mintBatch(owner, Object.keys(tokenIds), Object.values(tokenIds), '0x');
+      });
+
+      it('can receive ERC1155 safeTransfer', async function () {
+        await this.token.safeTransferFrom(
+          owner,
+          this.mock.address,
+          ...Object.entries(tokenIds)[0], // id + amount
+          '0x',
+          { from: owner },
+        );
+      });
+
+      it('can receive ERC1155 safeBatchTransfer', async function () {
+        await this.token.safeBatchTransferFrom(
+          owner,
+          this.mock.address,
+          Object.keys(tokenIds),
+          Object.values(tokenIds),
+          '0x',
+          { from: owner },
+        );
+      });
+    });
+  });
 });