diff --git a/.changeset/early-oranges-raise.md b/.changeset/early-oranges-raise.md new file mode 100644 index 00000000000..af60a443229 --- /dev/null +++ b/.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. diff --git a/contracts/token/ERC721/README.adoc b/contracts/token/ERC721/README.adoc index b3377afeff3..f571f5c57fa 100644 --- a/contracts/token/ERC721/README.adoc +++ b/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 <>) 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. diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol new file mode 100644 index 00000000000..87729188a5e --- /dev/null +++ b/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; + } +} diff --git a/docs/modules/ROOT/pages/governance.adoc b/docs/modules/ROOT/pages/governance.adoc index 358a63e909d..eb76321bfd8 100644 --- a/docs/modules/ROOT/pages/governance.adoc +++ b/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 diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index c0947957207..6867db31f84 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/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', diff --git a/test/token/ERC721/extensions/ERC721Wrapper.test.js b/test/token/ERC721/extensions/ERC721Wrapper.test.js new file mode 100644 index 00000000000..5c83d73cc47 --- /dev/null +++ b/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); + }); +});