diff --git a/CHANGELOG.md b/CHANGELOG.md index c83536aae8b..3d32db2b055 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ * `AccessControl`: new contract for managing permissions in a system, replacement for `Ownable` and `Roles`. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112)) ### Breaking changes + * `ERC721`: `burn(owner, tokenId)` was removed, use `burn(owner)` instead. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125)) + * `ERC721`: `_checkOnERC721Received` was removed. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125)) + * `PullPayment`, `Escrow`: `withdrawWithGas` was removed. The old `withdraw` function now forwards all gas. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125)) * `Roles` was removed, use `AccessControl` as a replacement. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112)) * `ECDSA`: when receiving an invalid signature, `recover` now reverts instead of returning the zero address. ([#2114](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2114)) * `Pausable`: moved to the `utils` directory. ([#2122](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2122)) diff --git a/contracts/mocks/ERC721Mock.sol b/contracts/mocks/ERC721Mock.sol index 1931afb64c7..00b8291169a 100644 --- a/contracts/mocks/ERC721Mock.sol +++ b/contracts/mocks/ERC721Mock.sol @@ -19,10 +19,6 @@ contract ERC721Mock is ERC721 { _mint(to, tokenId); } - function burn(address owner, uint256 tokenId) public { - _burn(owner, tokenId); - } - function burn(uint256 tokenId) public { _burn(tokenId); } diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 58bc099d0e7..3cd486fb073 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -28,35 +28,21 @@ contract PullPayment { } /** - * @dev Withdraw accumulated payments. + * @dev Withdraw accumulated payments, forwarding all gas to the recipient. * * Note that _any_ account can call this function, not just the `payee`. * This means that contracts unaware of the `PullPayment` protocol can still * receive funds this way, by having a separate account call * {withdrawPayments}. * - * NOTE: This function has been deprecated, use {withdrawPaymentsWithGas} - * instead. Calling contracts with fixed gas limits is an anti-pattern and - * may break contract interactions in network upgrades (hardforks). - * https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/[Learn more.] - * - * @param payee Whose payments will be withdrawn. - */ - function withdrawPayments(address payable payee) public virtual { - _escrow.withdraw(payee); - } - - /** - * @dev Same as {withdrawPayments}, but forwarding all gas to the recipient. - * * WARNING: Forwarding all gas opens the door to reentrancy vulnerabilities. * Make sure you trust the recipient, or are either following the * checks-effects-interactions pattern or using {ReentrancyGuard}. * - * _Available since v2.4.0._ + * @param payee Whose payments will be withdrawn. */ - function withdrawPaymentsWithGas(address payable payee) external virtual { - _escrow.withdrawWithGas(payee); + function withdrawPayments(address payable payee) public virtual { + _escrow.withdraw(payee); } /** diff --git a/contracts/payment/README.adoc b/contracts/payment/README.adoc index 716addbd604..56a9f80f446 100644 --- a/contracts/payment/README.adoc +++ b/contracts/payment/README.adoc @@ -1,6 +1,8 @@ = Payment -NOTE: This page is incomplete. We're working to improve it for the next release. Stay tuned! +Utilities related to sending and receiving payments. Examples are {PullPayment}, which implements the best security practices when sending funds to third parties, and {PaymentSplitter} to receive incoming payments among a number of beneficiaries. + +TIP: When transferring funds to and from untrusted third parties, there is always a security risk of reentrancy. If you would like to learn more about this and ways to protect against it, check out our blog post https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul]. == Utilities diff --git a/contracts/payment/escrow/Escrow.sol b/contracts/payment/escrow/Escrow.sol index dd1b6cf0d72..89cb410907a 100644 --- a/contracts/payment/escrow/Escrow.sol +++ b/contracts/payment/escrow/Escrow.sol @@ -42,36 +42,16 @@ contract Escrow is Ownable { } /** - * @dev Withdraw accumulated balance for a payee, forwarding 2300 gas (a - * Solidity `transfer`). - * - * NOTE: This function has been deprecated, use {withdrawWithGas} instead. - * Calling contracts with fixed-gas limits is an anti-pattern and may break - * contract interactions in network upgrades (hardforks). - * https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/[Learn more.] - * - * @param payee The address whose funds will be withdrawn and transferred to. - */ - function withdraw(address payable payee) public virtual onlyOwner { - uint256 payment = _deposits[payee]; - - _deposits[payee] = 0; - - payee.transfer(payment); - - emit Withdrawn(payee, payment); - } - - /** - * @dev Same as {withdraw}, but forwarding all gas to the recipient. + * @dev Withdraw accumulated balance for a payee, forwarding all gas to the + * recipient. * * WARNING: Forwarding all gas opens the door to reentrancy vulnerabilities. * Make sure you trust the recipient, or are either following the * checks-effects-interactions pattern or using {ReentrancyGuard}. * - * _Available since v2.4.0._ + * @param payee The address whose funds will be withdrawn and transferred to. */ - function withdrawWithGas(address payable payee) public virtual onlyOwner { + function withdraw(address payable payee) public virtual onlyOwner { uint256 payment = _deposits[payee]; _deposits[payee] = 0; diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index e2269948680..ea68f0aa893 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -269,12 +269,10 @@ contract ERC721 is Context, ERC165, IERC721 { /** * @dev Internal function to burn a specific token. * Reverts if the token does not exist. - * Deprecated, use {_burn} instead. - * @param owner owner of the token to burn * @param tokenId uint256 ID of the token being burned */ - function _burn(address owner, uint256 tokenId) internal virtual { - require(ownerOf(tokenId) == owner, "ERC721: burn of token that is not own"); + function _burn(uint256 tokenId) internal virtual { + address owner = ownerOf(tokenId); _beforeTokenTransfer(owner, address(0), tokenId); @@ -287,15 +285,6 @@ contract ERC721 is Context, ERC165, IERC721 { emit Transfer(owner, address(0), tokenId); } - /** - * @dev Internal function to burn a specific token. - * Reverts if the token does not exist. - * @param tokenId uint256 ID of the token being burned - */ - function _burn(uint256 tokenId) internal virtual { - _burn(ownerOf(tokenId), tokenId); - } - /** * @dev Internal function to transfer ownership of a given token ID to another address. * As opposed to {transferFrom}, this imposes no restrictions on msg.sender. @@ -324,7 +313,6 @@ contract ERC721 is Context, ERC165, IERC721 { * @dev Internal function to invoke {IERC721Receiver-onERC721Received} on a target address. * The call is not executed if the target address is not a contract. * - * This is an internal detail of the `ERC721` contract and its use is deprecated. * @param from address representing the previous owner of the given token ID * @param to target address that will receive the tokens * @param tokenId uint256 ID of the token to be transferred @@ -332,7 +320,7 @@ contract ERC721 is Context, ERC165, IERC721 { * @return bool whether the call correctly returned the expected magic value */ function _checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory _data) - internal virtual returns (bool) + private returns (bool) { if (!to.isContract()) { return true; diff --git a/test/payment/PullPayment.test.js b/test/payment/PullPayment.test.js index 758af3a8de2..3c2634b180f 100644 --- a/test/payment/PullPayment.test.js +++ b/test/payment/PullPayment.test.js @@ -46,16 +46,4 @@ describe('PullPayment', function () { expect(await balanceTracker.delta()).to.be.bignumber.equal(amount); expect(await this.contract.payments(payee1)).to.be.bignumber.equal('0'); }); - - it('can withdraw payment forwarding all gas', async function () { - const balanceTracker = await balance.tracker(payee1); - - await this.contract.callTransfer(payee1, amount, { from: payer }); - expect(await this.contract.payments(payee1)).to.be.bignumber.equal(amount); - - await this.contract.withdrawPaymentsWithGas(payee1); - - expect(await balanceTracker.delta()).to.be.bignumber.equal(amount); - expect(await this.contract.payments(payee1)).to.be.bignumber.equal('0'); - }); }); diff --git a/test/token/ERC721/ERC721.test.js b/test/token/ERC721/ERC721.test.js index b9709fd5a10..980f17322fb 100644 --- a/test/token/ERC721/ERC721.test.js +++ b/test/token/ERC721/ERC721.test.js @@ -9,7 +9,7 @@ const { shouldBehaveLikeERC721 } = require('./ERC721.behavior'); const ERC721Mock = contract.fromArtifact('ERC721Mock'); describe('ERC721', function () { - const [ owner, other ] = accounts; + const [ owner ] = accounts; beforeEach(async function () { this.token = await ERC721Mock.new(); @@ -47,54 +47,10 @@ describe('ERC721', function () { }); }); - describe('_burn(address, uint256)', function () { + describe('_burn', function () { it('reverts when burning a non-existent token id', async function () { await expectRevert( - this.token.methods['burn(address,uint256)'](owner, tokenId), 'ERC721: owner query for nonexistent token' - ); - }); - - context('with minted token', function () { - beforeEach(async function () { - await this.token.mint(owner, tokenId); - }); - - it('reverts when the account is not the owner', async function () { - await expectRevert( - this.token.methods['burn(address,uint256)'](other, tokenId), 'ERC721: burn of token that is not own' - ); - }); - - context('with burnt token', function () { - beforeEach(async function () { - ({ logs: this.logs } = await this.token.methods['burn(address,uint256)'](owner, tokenId)); - }); - - it('emits a Transfer event', function () { - expectEvent.inLogs(this.logs, 'Transfer', { from: owner, to: ZERO_ADDRESS, tokenId }); - }); - - it('deletes the token', async function () { - expect(await this.token.balanceOf(owner)).to.be.bignumber.equal('0'); - await expectRevert( - this.token.ownerOf(tokenId), 'ERC721: owner query for nonexistent token' - ); - }); - - it('reverts when burning a token id that has been deleted', async function () { - await expectRevert( - this.token.methods['burn(address,uint256)'](owner, tokenId), - 'ERC721: owner query for nonexistent token' - ); - }); - }); - }); - }); - - describe('_burn(uint256)', function () { - it('reverts when burning a non-existent token id', async function () { - await expectRevert( - this.token.methods['burn(uint256)'](tokenId), 'ERC721: owner query for nonexistent token' + this.token.burn(tokenId), 'ERC721: owner query for nonexistent token' ); }); @@ -105,7 +61,7 @@ describe('ERC721', function () { context('with burnt token', function () { beforeEach(async function () { - ({ logs: this.logs } = await this.token.methods['burn(uint256)'](tokenId)); + ({ logs: this.logs } = await this.token.burn(tokenId)); }); it('emits a Transfer event', function () { @@ -121,7 +77,7 @@ describe('ERC721', function () { it('reverts when burning a token id that has been deleted', async function () { await expectRevert( - this.token.methods['burn(uint256)'](tokenId), 'ERC721: owner query for nonexistent token' + this.token.burn(tokenId), 'ERC721: owner query for nonexistent token' ); }); });