Add validation in Governor on ERC-721 or ERC-1155 received (#4314)

Co-authored-by: Hadrien Croubois <[email protected]> Co-authored-by: Francisco <[email protected]> Co-authored-by: Ernesto García <[email protected]>
OpenZeppelin · Jun 16, 2023 · cd48b3e · cd48b3e
1 parent 6724873
commit cd48b3e
Show file tree

Hide file tree

Showing 8 changed files with 306 additions and 37 deletions.
diff --git a/.changeset/mighty-donuts-smile.md b/.changeset/mighty-donuts-smile.md
@@ -0,0 +1,6 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`Governor`: Add validation in ERC1155 and ERC721 receiver hooks to ensure Governor is the executor.
+
diff --git a/.changeset/thin-camels-matter.md b/.changeset/thin-camels-matter.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`ERC1155`: Bubble errors triggered in the `onERC1155Received` and `onERC1155BatchReceived` hooks.
diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
@@ -70,7 +70,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
      * governance protocol (since v4.6).
      */
     modifier onlyGovernance() {
-        if (_msgSender() != _executor()) {
+        if (_executor() != _msgSender()) {
             revert GovernorOnlyExecutor(_msgSender());
         }
         if (_executor() != address(this)) {
@@ -631,20 +631,29 @@ 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 returns (bytes4) {
+        if (_executor() != address(this)) {
+            revert GovernorDisabledDeposit();
+        }
         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, address, uint256, uint256, bytes memory) public virtual returns (bytes4) {
+        if (_executor() != address(this)) {
+            revert GovernorDisabledDeposit();
+        }
         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,
@@ -653,6 +662,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
         uint256[] memory,
         bytes memory
     ) public virtual returns (bytes4) {
+        if (_executor() != address(this)) {
+            revert GovernorDisabledDeposit();
+        }
         return this.onERC1155BatchReceived.selector;
     }
 

diff --git a/contracts/mocks/token/ERC1155ReceiverMock.sol b/contracts/mocks/token/ERC1155ReceiverMock.sol
@@ -6,15 +6,24 @@ import "../../token/ERC1155/IERC1155Receiver.sol";
 import "../../utils/introspection/ERC165.sol";
 
 contract ERC1155ReceiverMock is ERC165, IERC1155Receiver {
+    enum RevertType {
+        None,
+        Empty,
+        String,
+        Custom
+    }
+
     bytes4 private _recRetval;
-    bool private _recReverts;
+    RevertType private _recReverts;
     bytes4 private _batRetval;
-    bool private _batReverts;
+    RevertType private _batReverts;
 
     event Received(address operator, address from, uint256 id, uint256 value, bytes data, uint256 gas);
     event BatchReceived(address operator, address from, uint256[] ids, uint256[] values, bytes data, uint256 gas);
 
-    constructor(bytes4 recRetval, bool recReverts, bytes4 batRetval, bool batReverts) {
+    error ERC1155ReceiverMockError();
+
+    constructor(bytes4 recRetval, RevertType recReverts, bytes4 batRetval, RevertType batReverts) {
         _recRetval = recRetval;
         _recReverts = recReverts;
         _batRetval = batRetval;
@@ -28,7 +37,14 @@ contract ERC1155ReceiverMock is ERC165, IERC1155Receiver {
         uint256 value,
         bytes calldata data
     ) external returns (bytes4) {
-        require(!_recReverts, "ERC1155ReceiverMock: reverting on receive");
+        if (_recReverts == RevertType.Empty) {
+            revert();
+        } else if (_recReverts == RevertType.String) {
+            revert("ERC1155ReceiverMock: reverting on receive");
+        } else if (_recReverts == RevertType.Custom) {
+            revert ERC1155ReceiverMockError();
+        }
+
         emit Received(operator, from, id, value, data, gasleft());
         return _recRetval;
     }
@@ -40,7 +56,14 @@ contract ERC1155ReceiverMock is ERC165, IERC1155Receiver {
         uint256[] calldata values,
         bytes calldata data
     ) external returns (bytes4) {
-        require(!_batReverts, "ERC1155ReceiverMock: reverting on batch receive");
+        if (_batReverts == RevertType.Empty) {
+            revert();
+        } else if (_batReverts == RevertType.String) {
+            revert("ERC1155ReceiverMock: reverting on batch receive");
+        } else if (_batReverts == RevertType.Custom) {
+            revert ERC1155ReceiverMockError();
+        }
+
         emit BatchReceived(operator, from, ids, values, data, gasleft());
         return _batRetval;
     }

diff --git a/contracts/token/ERC1155/ERC1155.sol b/contracts/token/ERC1155/ERC1155.sol
@@ -364,11 +364,16 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER
                     // Tokens rejected
                     revert ERC1155InvalidReceiver(to);
                 }
-            } catch Error(string memory reason) {
-                revert(reason);
-            } catch {
-                // non-ERC1155Receiver implementer
-                revert ERC1155InvalidReceiver(to);
+            } catch (bytes memory reason) {
+                if (reason.length == 0) {
+                    // non-ERC1155Receiver implementer
+                    revert ERC1155InvalidReceiver(to);
+                } else {
+                    /// @solidity memory-safe-assembly
+                    assembly {
+                        revert(add(32, reason), mload(reason))
+                    }
+                }
             }
         }
     }
@@ -389,11 +394,16 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER
                     // Tokens rejected
                     revert ERC1155InvalidReceiver(to);
                 }
-            } catch Error(string memory reason) {
-                revert(reason);
-            } catch {
-                // non-ERC1155Receiver implementer
-                revert ERC1155InvalidReceiver(to);
+            } catch (bytes memory reason) {
+                if (reason.length == 0) {
+                    // non-ERC1155Receiver implementer
+                    revert ERC1155InvalidReceiver(to);
+                } else {
+                    /// @solidity memory-safe-assembly
+                    assembly {
+                        revert(add(32, reason), mload(reason))
+                    }
+                }
             }
         }
     }

diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js
@@ -11,6 +11,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(
@@ -212,6 +214,70 @@ contract('GovernorTimelockCompound', function (accounts) {
             ]);
           });
         });
+
+        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 expectRevertCustomError(
+                this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }),
+                'GovernorDisabledDeposit',
+                [],
+              );
+            });
+          });
+
+          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 expectRevertCustomError(
+                this.token.safeTransferFrom(
+                  owner,
+                  this.mock.address,
+                  ...Object.entries(tokenIds)[0], // id + amount
+                  '0x',
+                  { from: owner },
+                ),
+                'GovernorDisabledDeposit',
+                [],
+              );
+            });
+
+            it("can't receive ERC1155 safeBatchTransfer", async function () {
+              await expectRevertCustomError(
+                this.token.safeBatchTransferFrom(
+                  owner,
+                  this.mock.address,
+                  Object.keys(tokenIds),
+                  Object.values(tokenIds),
+                  '0x',
+                  { from: owner },
+                ),
+                'GovernorDisabledDeposit',
+                [],
+              );
+            });
+          });
+        });
       });
 
       describe('cancel', function () {

diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js
@@ -10,6 +10,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' },
@@ -412,6 +414,70 @@ contract('GovernorTimelockControl', function (accounts) {
             expect(await this.mock.timelock()).to.be.bignumber.equal(this.newTimelock.address);
           });
         });
+
+        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 expectRevertCustomError(
+                this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }),
+                'GovernorDisabledDeposit',
+                [],
+              );
+            });
+          });
+
+          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 expectRevertCustomError(
+                this.token.safeTransferFrom(
+                  owner,
+                  this.mock.address,
+                  ...Object.entries(tokenIds)[0], // id + amount
+                  '0x',
+                  { from: owner },
+                ),
+                'GovernorDisabledDeposit',
+                [],
+              );
+            });
+
+            it("can't receive ERC1155 safeBatchTransfer", async function () {
+              await expectRevertCustomError(
+                this.token.safeBatchTransferFrom(
+                  owner,
+                  this.mock.address,
+                  Object.keys(tokenIds),
+                  Object.values(tokenIds),
+                  '0x',
+                  { from: owner },
+                ),
+                'GovernorDisabledDeposit',
+                [],
+              );
+            });
+          });
+        });
       });
 
       it('clear queue of pending governor calls', async function () {