Add ERC721 Wrapper (#3863)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>

.changeset/early-oranges-raise.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`ERC721Wrapper`: add a new extension of the `ERC721` token which wraps an underlying token. Deposit and withdraw guarantee that the ownership of each token is backed by a corresponding underlying token with the same identifier.

contracts/token/ERC721/README.adoc

@@ -28,6 +28,7 @@ Additionally there are a few of other extensions:
 * {ERC721Royalty}: A way to signal royalty information following ERC2981.
 * {ERC721Pausable}: A primitive to pause contract operation.
 * {ERC721Burnable}: A way for token holders to burn their own tokens.
+* {ERC721Wrapper}: Wrapper to create an ERC721 backed by another ERC721, with deposit and withdraw methods. Useful in conjunction with {ERC721Votes}.
 
 NOTE: This core set of contracts is designed to be unopinionated, allowing developers to access the internal functions in ERC721 (such as <<ERC721-_mint-address-uint256-,`_mint`>>) and expose them as external functions in the way they prefer. On the other hand, xref:ROOT:erc721.adoc#Presets[ERC721 Presets] (such as {ERC721PresetMinterPauserAutoId}) are designed using opinionated patterns to provide developers with ready to use, deployable contracts.
 
@@ -59,6 +60,8 @@ NOTE: This core set of contracts is designed to be unopinionated, allowing devel
 
 {{ERC721Royalty}}
 
+{{ERC721Wrapper}}
+
 == Presets
 
 These contracts are preconfigured combinations of the above features. They can be used through inheritance or as models to copy and paste their source code.

contracts/token/ERC721/extensions/ERC721Wrapper.sol

@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.0;
+
+import "../ERC721.sol";
+import "../utils/ERC721Holder.sol";
+
+/**
+ * @dev Extension of the ERC721 token contract to support token wrapping.
+ *
+ * Users can deposit and withdraw an "underlying token" and receive a "wrapped token" with a matching tokenId. This is useful
+ * in conjunction with other modules. For example, combining this wrapping mechanism with {ERC721Votes} will allow the
+ * wrapping of an existing "basic" ERC721 into a governance token.
+ *
+ * _Available since v4.9.0_
+ */
+abstract contract ERC721Wrapper is ERC721, ERC721Holder {
+    IERC721 private immutable _underlying;
+
+    // Kept as bytes12 so it can be packed with an address
+    // Equal to 0xb125e89df18e2ceac5fd2fa8
+    bytes12 public constant WRAPPER_ACCEPT_MAGIC = bytes12(keccak256("WRAPPER_ACCEPT_MAGIC"));
+
+    constructor(IERC721 underlyingToken) {
+        _underlying = underlyingToken;
+    }
+
+    /**
+     * @dev Allow a user to deposit underlying tokens and mint the corresponding tokenIds.
+     */
+    function depositFor(address account, uint256[] memory tokenIds) public virtual returns (bool) {
+        bytes memory data = abi.encodePacked(WRAPPER_ACCEPT_MAGIC, account);
+
+        uint256 length = tokenIds.length;
+        for (uint256 i = 0; i < length; ++i) {
+            underlying().safeTransferFrom(_msgSender(), address(this), tokenIds[i], data);
+        }
+
+        return true;
+    }
+
+    /**
+     * @dev Allow a user to burn wrapped tokens and withdraw the corresponding tokenIds of the underlying tokens.
+     */
+    function withdrawTo(address account, uint256[] memory tokenIds) public virtual returns (bool) {
+        uint256 length = tokenIds.length;
+        for (uint256 i = 0; i < length; ++i) {
+            uint256 tokenId = tokenIds[i];
+            require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721Wrapper: caller is not token owner or approved");
+            _burn(tokenId);
+            // Checks were already performed at this point, and there's no way to retake ownership or approval from
+            // the wrapped tokenId after this point, so it's safe to remove the reentrancy check for the next line.
+            // slither-disable-next-line reentrancy-no-eth
+            underlying().safeTransferFrom(address(this), account, tokenId);
+        }
+
+        return true;
+    }
+
+    /**
+     * @dev Overrides {IERC721Receiver-onERC721Received} to allow minting on direct ERC721 transfers to
+     * this contract.
+     *
+     * In case there's data attached, it validates that the sender is aware of this contract's existence and behavior
+     * by checking a magic value (`WRAPPER_ACCEPT_MAGIC`) in the first 12 bytes. If it also matches, the rest 20
+     * bytes are used as an address to send the tokens to.
+     *
+     * WARNING: Doesn't work with unsafe transfers (eg. {IERC721-transferFrom}). Use {ERC721Wrapper-_recover}
+     * for recovering in that scenario.
+     */
+    function onERC721Received(
+        address,
+        address from,
+        uint256 tokenId,
+        bytes memory data
+    ) public override returns (bytes4) {
+        require(address(underlying()) == _msgSender(), "ERC721Wrapper: caller is not underlying");
+        if (data.length > 0) {
+            require(data.length == 32 && WRAPPER_ACCEPT_MAGIC == bytes12(data), "ERC721Wrapper: Invalid data format");
+            from = address(bytes20(bytes32(data) << 96));
+        }
+        _safeMint(from, tokenId);
+        return IERC721Receiver.onERC721Received.selector;
+    }
+
+    /**
+     * @dev Mint a wrapped token to cover any underlyingToken that would have been transferred by mistake. Internal
+     * function that can be exposed with access control if desired.
+     */
+    function _recover(address account, uint256 tokenId) internal virtual returns (uint256) {
+        require(underlying().ownerOf(tokenId) == address(this), "ERC721Wrapper: wrapper is not token owner");
+        _safeMint(account, tokenId);
+        return tokenId;
+    }
+
+    /**
+     * @dev Returns the underlying token.
+     */
+    function underlying() public view virtual returns (IERC721) {
+        return _underlying;
+    }
+}

docs/modules/ROOT/pages/governance.adoc

@@ -119,7 +119,7 @@ contract MyToken is ERC20, ERC20Permit, ERC20Votes, ERC20Wrapper {
 }
 ```
 
-NOTE: Voting power could be determined in different ways: multiple ERC20 tokens, ERC721 tokens, sybil resistant identities, etc. All of these options are potentially supported by writing a custom Votes module for your Governor. The only other source of voting power available in OpenZeppelin Contracts currently is xref:api:token/ERC721.adoc#ERC721Votes[`ERC721Votes`].
+NOTE:The only other source of voting power available in OpenZeppelin Contracts currently is xref:api:token/ERC721.adoc#ERC721Votes[`ERC721Votes`]. ERC721 tokens that don't provide this functionality can be wrapped into a voting tokens using a combination of xref:api:token/ERC721.adoc#ERC721Votes[`ERC721Votes`] and xref:api:token/ERC721Wrapper.adoc#ERC721Wrapper[`ERC721Wrapper`].
 
 === Governor
 

test/token/ERC721/ERC721.behavior.js

@@ -5,6 +5,7 @@ const { ZERO_ADDRESS } = constants;
 const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsInterface.behavior');
 
 const ERC721ReceiverMock = artifacts.require('ERC721ReceiverMock');
+const NonERC721ReceiverMock = artifacts.require('CallReceiverMock');
 
 const Error = ['None', 'RevertWithMessage', 'RevertWithoutMessage', 'Panic'].reduce(
   (acc, entry, idx) => Object.assign({ [entry]: idx }, acc),
@@ -316,7 +317,7 @@ function shouldBehaveLikeERC721(errorPrefix, owner, newOwner, approved, anotherA
 
         describe('to a contract that does not implement the required function', function () {
           it('reverts', async function () {
-            const nonReceiver = this.token;
+            const nonReceiver = await NonERC721ReceiverMock.new();
             await expectRevert(
               this.token.safeTransferFrom(owner, nonReceiver.address, tokenId, { from: owner }),
               'ERC721: transfer to non ERC721Receiver implementer',
@@ -392,7 +393,7 @@ function shouldBehaveLikeERC721(errorPrefix, owner, newOwner, approved, anotherA
 
         context('to a contract that does not implement the required function', function () {
           it('reverts', async function () {
-            const nonReceiver = this.token;
+            const nonReceiver = await NonERC721ReceiverMock.new();
             await expectRevert(
               this.token.$_safeMint(nonReceiver.address, tokenId),
               'ERC721: transfer to non ERC721Receiver implementer',

test/token/ERC721/extensions/ERC721Wrapper.test.js

@@ -0,0 +1,338 @@
+const { BN, expectEvent, constants, expectRevert } = require('@openzeppelin/test-helpers');
+const { expect } = require('chai');
+const { keccakFromString, bufferToHex } = require('ethereumjs-util');
+
+const { shouldBehaveLikeERC721 } = require('../ERC721.behavior');
+
+const ERC721 = artifacts.require('$ERC721');
+const ERC721Wrapper = artifacts.require('$ERC721Wrapper');
+
+contract('ERC721Wrapper', function (accounts) {
+  const [initialHolder, anotherAccount, approvedAccount] = accounts;
+
+  const name = 'My Token';
+  const symbol = 'MTKN';
+  const firstTokenId = new BN(1);
+  const secondTokenId = new BN(2);
+
+  beforeEach(async function () {
+    this.underlying = await ERC721.new(name, symbol);
+    this.token = await ERC721Wrapper.new(`Wrapped ${name}`, `W${symbol}`, this.underlying.address);
+
+    await this.underlying.$_safeMint(initialHolder, firstTokenId);
+    await this.underlying.$_safeMint(initialHolder, secondTokenId);
+  });
+
+  it('has a name', async function () {
+    expect(await this.token.name()).to.equal(`Wrapped ${name}`);
+  });
+
+  it('has a symbol', async function () {
+    expect(await this.token.symbol()).to.equal(`W${symbol}`);
+  });
+
+  it('has underlying', async function () {
+    expect(await this.token.underlying()).to.be.bignumber.equal(this.underlying.address);
+  });
+
+  describe('depositFor', function () {
+    it('works with token approval', async function () {
+      await this.underlying.approve(this.token.address, firstTokenId, { from: initialHolder });
+
+      const { tx } = await this.token.depositFor(initialHolder, [firstTokenId], { from: initialHolder });
+
+      await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
+        from: initialHolder,
+        to: this.token.address,
+        tokenId: firstTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: constants.ZERO_ADDRESS,
+        to: initialHolder,
+        tokenId: firstTokenId,
+      });
+    });
+
+    it('works with approval for all', async function () {
+      await this.underlying.setApprovalForAll(this.token.address, true, { from: initialHolder });
+
+      const { tx } = await this.token.depositFor(initialHolder, [firstTokenId], { from: initialHolder });
+
+      await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
+        from: initialHolder,
+        to: this.token.address,
+        tokenId: firstTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: constants.ZERO_ADDRESS,
+        to: initialHolder,
+        tokenId: firstTokenId,
+      });
+    });
+
+    it('works sending to another account', async function () {
+      await this.underlying.approve(this.token.address, firstTokenId, { from: initialHolder });
+
+      const { tx } = await this.token.depositFor(anotherAccount, [firstTokenId], { from: initialHolder });
+
+      await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
+        from: initialHolder,
+        to: this.token.address,
+        tokenId: firstTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: constants.ZERO_ADDRESS,
+        to: anotherAccount,
+        tokenId: firstTokenId,
+      });
+    });
+
+    it('works with multiple tokens', async function () {
+      await this.underlying.approve(this.token.address, firstTokenId, { from: initialHolder });
+      await this.underlying.approve(this.token.address, secondTokenId, { from: initialHolder });
+
+      const { tx } = await this.token.depositFor(initialHolder, [firstTokenId, secondTokenId], { from: initialHolder });
+
+      await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
+        from: initialHolder,
+        to: this.token.address,
+        tokenId: firstTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: constants.ZERO_ADDRESS,
+        to: initialHolder,
+        tokenId: firstTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
+        from: initialHolder,
+        to: this.token.address,
+        tokenId: secondTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: constants.ZERO_ADDRESS,
+        to: initialHolder,
+        tokenId: secondTokenId,
+      });
+    });
+
+    it('reverts with missing approval', async function () {
+      await expectRevert(
+        this.token.depositFor(initialHolder, [firstTokenId], { from: initialHolder }),
+        'ERC721: caller is not token owner or approved',
+      );
+    });
+  });
+
+  describe('withdrawTo', function () {
+    beforeEach(async function () {
+      await this.underlying.approve(this.token.address, firstTokenId, { from: initialHolder });
+      await this.token.depositFor(initialHolder, [firstTokenId], { from: initialHolder });
+    });
+
+    it('works for an owner', async function () {
+      const { tx } = await this.token.withdrawTo(initialHolder, [firstTokenId], { from: initialHolder });
+
+      await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
+        from: this.token.address,
+        to: initialHolder,
+        tokenId: firstTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: initialHolder,
+        to: constants.ZERO_ADDRESS,
+        tokenId: firstTokenId,
+      });
+    });
+
+    it('works for an approved', async function () {
+      await this.token.approve(approvedAccount, firstTokenId, { from: initialHolder });
+
+      const { tx } = await this.token.withdrawTo(initialHolder, [firstTokenId], { from: approvedAccount });
+
+      await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
+        from: this.token.address,
+        to: initialHolder,
+        tokenId: firstTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: initialHolder,
+        to: constants.ZERO_ADDRESS,
+        tokenId: firstTokenId,
+      });
+    });
+
+    it('works for an approved for all', async function () {
+      await this.token.setApprovalForAll(approvedAccount, true, { from: initialHolder });
+
+      const { tx } = await this.token.withdrawTo(initialHolder, [firstTokenId], { from: approvedAccount });
+
+      await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
+        from: this.token.address,
+        to: initialHolder,
+        tokenId: firstTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: initialHolder,
+        to: constants.ZERO_ADDRESS,
+        tokenId: firstTokenId,
+      });
+    });
+
+    it("doesn't work for a non-owner nor approved", async function () {
+      await expectRevert(
+        this.token.withdrawTo(initialHolder, [firstTokenId], { from: anotherAccount }),
+        'ERC721Wrapper: caller is not token owner or approved',
+      );
+    });
+
+    it('works with multiple tokens', async function () {
+      await this.underlying.approve(this.token.address, secondTokenId, { from: initialHolder });
+      await this.token.depositFor(initialHolder, [secondTokenId], { from: initialHolder });
+
+      const { tx } = await this.token.withdrawTo(initialHolder, [firstTokenId, secondTokenId], { from: initialHolder });
+
+      await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
+        from: this.token.address,
+        to: initialHolder,
+        tokenId: firstTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: initialHolder,
+        to: constants.ZERO_ADDRESS,
+        tokenId: firstTokenId,
+      });
+    });
+
+    it('works to another account', async function () {
+      const { tx } = await this.token.withdrawTo(anotherAccount, [firstTokenId], { from: initialHolder });
+
+      await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
+        from: this.token.address,
+        to: anotherAccount,
+        tokenId: firstTokenId,
+      });
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: initialHolder,
+        to: constants.ZERO_ADDRESS,
+        tokenId: firstTokenId,
+      });
+    });
+  });
+
+  describe('onERC721Received', function () {
+    const WRAPPER_ACCEPT_MAGIC = bufferToHex(keccakFromString('WRAPPER_ACCEPT_MAGIC')).slice(0, 26); // Include 0x
+
+    const magicWithAddresss = address =>
+      web3.utils.encodePacked(
+        {
+          value: WRAPPER_ACCEPT_MAGIC,
+          type: 'bytes12',
+        },
+        {
+          value: address,
+          type: 'address',
+        },
+      );
+
+    it('only allows calls from underlying', async function () {
+      await expectRevert(
+        this.token.onERC721Received(
+          initialHolder,
+          this.token.address,
+          firstTokenId,
+          magicWithAddresss(anotherAccount), // Correct data
+          { from: anotherAccount },
+        ),
+        'ERC721Wrapper: caller is not underlying',
+      );
+    });
+
+    describe('when data length is > 0', function () {
+      it('reverts with arbitrary data', async function () {
+        await expectRevert(
+          this.underlying.methods['safeTransferFrom(address,address,uint256,bytes)'](
+            initialHolder,
+            this.token.address,
+            firstTokenId,
+            '0x0123',
+            {
+              from: initialHolder,
+            },
+          ),
+          'ERC721Wrapper: Invalid data format',
+        );
+      });
+
+      it('reverts with the magic value and data length different to 32', async function () {
+        await expectRevert(
+          this.underlying.methods['safeTransferFrom(address,address,uint256,bytes)'](
+            initialHolder,
+            this.token.address,
+            firstTokenId,
+            WRAPPER_ACCEPT_MAGIC, // Reverts for any non-32 bytes value
+            {
+              from: initialHolder,
+            },
+          ),
+          'ERC721Wrapper: Invalid data format',
+        );
+      });
+
+      it('mints token to specific holder with address after magic value', async function () {
+        const { tx } = await this.underlying.methods['safeTransferFrom(address,address,uint256,bytes)'](
+          initialHolder,
+          this.token.address,
+          firstTokenId,
+          magicWithAddresss(anotherAccount),
+          {
+            from: initialHolder,
+          },
+        );
+
+        await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+          from: constants.ZERO_ADDRESS,
+          to: anotherAccount,
+          tokenId: firstTokenId,
+        });
+      });
+    });
+
+    it('mints a token to from if no data is specified', async function () {
+      const { tx } = await this.underlying.safeTransferFrom(initialHolder, this.token.address, firstTokenId, {
+        from: initialHolder,
+      });
+
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: constants.ZERO_ADDRESS,
+        to: initialHolder,
+        tokenId: firstTokenId,
+      });
+    });
+  });
+
+  describe('_recover', function () {
+    it('works if there is something to recover', async function () {
+      // Should use `transferFrom` to avoid `onERC721Received` minting
+      await this.underlying.transferFrom(initialHolder, this.token.address, firstTokenId, { from: initialHolder });
+
+      const { tx } = await this.token.$_recover(anotherAccount, firstTokenId);
+
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: constants.ZERO_ADDRESS,
+        to: anotherAccount,
+        tokenId: firstTokenId,
+      });
+    });
+
+    it('reverts if there is nothing to recover', async function () {
+      await expectRevert(
+        this.token.$_recover(initialHolder, firstTokenId),
+        'ERC721Wrapper: wrapper is not token owner',
+      );
+    });
+  });
+
+  describe('ERC712 behavior', function () {
+    shouldBehaveLikeERC721('ERC721', ...accounts);
+  });
+});