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

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco <fg@frang.io>
Co-authored-by: Ernesto García <ernestognw@gmail.com>

.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.
+

.changeset/thin-camels-matter.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`ERC1155`: Bubble errors triggered in the `onERC1155Received` and `onERC1155BatchReceived` hooks.

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;
     }
 

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;
     }

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))
+                    }
+                }
             }
         }
     }

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 () {

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 () {

test/token/ERC1155/ERC1155.behavior.js

@@ -5,8 +5,10 @@ const { expect } = require('chai');
 
 const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsInterface.behavior');
 const { expectRevertCustomError } = require('../../helpers/customError');
+const { Enum } = require('../../helpers/enums');
 
 const ERC1155ReceiverMock = artifacts.require('ERC1155ReceiverMock');
+const RevertType = Enum('None', 'Empty', 'String', 'Custom');
 
 function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, multiTokenHolder, recipient, proxy]) {
   const firstTokenId = new BN(1);
@@ -296,9 +298,9 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m
         beforeEach(async function () {
           this.receiver = await ERC1155ReceiverMock.new(
             RECEIVER_SINGLE_MAGIC_VALUE,
-            false,
+            RevertType.None,
             RECEIVER_BATCH_MAGIC_VALUE,
-            false,
+            RevertType.None,
           );
         });
 
@@ -370,7 +372,12 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m
 
       context('to a receiver contract returning unexpected value', function () {
         beforeEach(async function () {
-          this.receiver = await ERC1155ReceiverMock.new('0x00c0ffee', false, RECEIVER_BATCH_MAGIC_VALUE, false);
+          this.receiver = await ERC1155ReceiverMock.new(
+            '0x00c0ffee',
+            RevertType.None,
+            RECEIVER_BATCH_MAGIC_VALUE,
+            RevertType.None,
+          );
         });
 
         it('reverts', async function () {
@@ -385,23 +392,55 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m
       });
 
       context('to a receiver contract that reverts', function () {
-        beforeEach(async function () {
-          this.receiver = await ERC1155ReceiverMock.new(
+        it('with empty reason', async function () {
+          const receiver = await ERC1155ReceiverMock.new(
             RECEIVER_SINGLE_MAGIC_VALUE,
-            true,
+            RevertType.Empty,
             RECEIVER_BATCH_MAGIC_VALUE,
-            false,
+            RevertType.None,
+          );
+
+          await expectRevertCustomError(
+            this.token.safeTransferFrom(multiTokenHolder, receiver.address, firstTokenId, firstAmount, '0x', {
+              from: multiTokenHolder,
+            }),
+            'ERC1155InvalidReceiver',
+            [receiver.address],
           );
         });
 
-        it('reverts', async function () {
+        it('with reason string', async function () {
+          const receiver = await ERC1155ReceiverMock.new(
+            RECEIVER_SINGLE_MAGIC_VALUE,
+            RevertType.String,
+            RECEIVER_BATCH_MAGIC_VALUE,
+            RevertType.None,
+          );
+
           await expectRevert(
-            this.token.safeTransferFrom(multiTokenHolder, this.receiver.address, firstTokenId, firstAmount, '0x', {
+            this.token.safeTransferFrom(multiTokenHolder, receiver.address, firstTokenId, firstAmount, '0x', {
               from: multiTokenHolder,
             }),
             'ERC1155ReceiverMock: reverting on receive',
           );
         });
+
+        it('with custom error', async function () {
+          const receiver = await ERC1155ReceiverMock.new(
+            RECEIVER_SINGLE_MAGIC_VALUE,
+            RevertType.Custom,
+            RECEIVER_BATCH_MAGIC_VALUE,
+            RevertType.None,
+          );
+
+          await expectRevertCustomError(
+            this.token.safeTransferFrom(multiTokenHolder, receiver.address, firstTokenId, firstAmount, '0x', {
+              from: multiTokenHolder,
+            }),
+            'ERC1155ReceiverMockError',
+            [],
+          );
+        });
       });
 
       context('to a contract that does not implement the required function', function () {
@@ -590,9 +629,9 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m
         beforeEach(async function () {
           this.receiver = await ERC1155ReceiverMock.new(
             RECEIVER_SINGLE_MAGIC_VALUE,
-            false,
+            RevertType.None,
             RECEIVER_BATCH_MAGIC_VALUE,
-            false,
+            RevertType.None,
           );
         });
 
@@ -666,9 +705,9 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m
         beforeEach(async function () {
           this.receiver = await ERC1155ReceiverMock.new(
             RECEIVER_SINGLE_MAGIC_VALUE,
-            false,
+            RevertType.None,
             RECEIVER_SINGLE_MAGIC_VALUE,
-            false,
+            RevertType.None,
           );
         });
 
@@ -689,20 +728,40 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m
       });
 
       context('to a receiver contract that reverts', function () {
-        beforeEach(async function () {
-          this.receiver = await ERC1155ReceiverMock.new(
+        it('with empty reason', async function () {
+          const receiver = await ERC1155ReceiverMock.new(
             RECEIVER_SINGLE_MAGIC_VALUE,
-            false,
+            RevertType.None,
             RECEIVER_BATCH_MAGIC_VALUE,
-            true,
+            RevertType.Empty,
+          );
+
+          await expectRevertCustomError(
+            this.token.safeBatchTransferFrom(
+              multiTokenHolder,
+              receiver.address,
+              [firstTokenId, secondTokenId],
+              [firstAmount, secondAmount],
+              '0x',
+              { from: multiTokenHolder },
+            ),
+            'ERC1155InvalidReceiver',
+            [receiver.address],
           );
         });
 
-        it('reverts', async function () {
+        it('with reason string', async function () {
+          const receiver = await ERC1155ReceiverMock.new(
+            RECEIVER_SINGLE_MAGIC_VALUE,
+            RevertType.None,
+            RECEIVER_BATCH_MAGIC_VALUE,
+            RevertType.String,
+          );
+
           await expectRevert(
             this.token.safeBatchTransferFrom(
               multiTokenHolder,
-              this.receiver.address,
+              receiver.address,
               [firstTokenId, secondTokenId],
               [firstAmount, secondAmount],
               '0x',
@@ -711,15 +770,37 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m
             'ERC1155ReceiverMock: reverting on batch receive',
           );
         });
+
+        it('with custom error', async function () {
+          const receiver = await ERC1155ReceiverMock.new(
+            RECEIVER_SINGLE_MAGIC_VALUE,
+            RevertType.None,
+            RECEIVER_BATCH_MAGIC_VALUE,
+            RevertType.Custom,
+          );
+
+          await expectRevertCustomError(
+            this.token.safeBatchTransferFrom(
+              multiTokenHolder,
+              receiver.address,
+              [firstTokenId, secondTokenId],
+              [firstAmount, secondAmount],
+              '0x',
+              { from: multiTokenHolder },
+            ),
+            'ERC1155ReceiverMockError',
+            [],
+          );
+        });
       });
 
       context('to a receiver contract that reverts only on single transfers', function () {
         beforeEach(async function () {
           this.receiver = await ERC1155ReceiverMock.new(
             RECEIVER_SINGLE_MAGIC_VALUE,
-            true,
+            RevertType.String,
             RECEIVER_BATCH_MAGIC_VALUE,
-            false,
+            RevertType.None,
           );
 
           this.toWhom = this.receiver.address;