From bb51b07afa8c49dd0e67a019ce4d256fd01e624f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 28 Oct 2019 18:01:38 -0300 Subject: [PATCH 1/8] Add withdrawWithGas --- contracts/payment/PullPayment.sol | 11 +++++++++++ contracts/payment/escrow/Escrow.sol | 19 +++++++++++++++++++ test/payment/PullPayment.test.js | 12 ++++++++++++ 3 files changed, 42 insertions(+) diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 594d618f2dd..7cdd5bb6b78 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -36,6 +36,17 @@ contract PullPayment { _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}. + */ + function withdrawPaymentsWithGas(address payable payee) public { + _escrow.withdrawWithGas(payee); + } + /** * @dev Returns the payments owed to an address. * @param dest The creditor's address. diff --git a/contracts/payment/escrow/Escrow.sol b/contracts/payment/escrow/Escrow.sol index 4422d328e87..d62948affd9 100644 --- a/contracts/payment/escrow/Escrow.sol +++ b/contracts/payment/escrow/Escrow.sol @@ -2,6 +2,7 @@ pragma solidity ^0.5.0; import "../../math/SafeMath.sol"; import "../../ownership/Secondary.sol"; +import "../../utils/Address.sol"; /** * @title Escrow @@ -18,6 +19,7 @@ import "../../ownership/Secondary.sol"; */ contract Escrow is Secondary { using SafeMath for uint256; + using Address for address payable; event Deposited(address indexed payee, uint256 weiAmount); event Withdrawn(address indexed payee, uint256 weiAmount); @@ -52,4 +54,21 @@ contract Escrow is Secondary { emit Withdrawn(payee, payment); } + + /** + * @dev Same as {withdraw}, 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}. + */ + function withdrawWithGas(address payable payee) public onlyPrimary { + uint256 payment = _deposits[payee]; + + _deposits[payee] = 0; + + payee.sendValue(payment); + + emit Withdrawn(payee, payment); + } } diff --git a/test/payment/PullPayment.test.js b/test/payment/PullPayment.test.js index 7f428ff28d4..ffeffc1dead 100644 --- a/test/payment/PullPayment.test.js +++ b/test/payment/PullPayment.test.js @@ -42,4 +42,16 @@ contract('PullPayment', function ([_, payer, payee1, payee2]) { (await balanceTracker.delta()).should.be.bignumber.equal(amount); (await this.contract.payments(payee1)).should.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 }); + (await this.contract.payments(payee1)).should.be.bignumber.equal(amount); + + await this.contract.withdrawPaymentsWithGas(payee1); + + (await balanceTracker.delta()).should.be.bignumber.equal(amount); + (await this.contract.payments(payee1)).should.be.bignumber.equal('0'); + }); }); From 57b3861a3382f1d13bcf304b67f744797836c0dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 28 Oct 2019 18:12:32 -0300 Subject: [PATCH 2/8] Improve docs --- contracts/payment/PullPayment.sol | 9 +++++++++ contracts/payment/escrow/Escrow.sol | 9 ++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 7cdd5bb6b78..3628f877a4e 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -31,6 +31,11 @@ contract PullPayment { * 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.] */ function withdrawPayments(address payable payee) public { _escrow.withdraw(payee); @@ -39,6 +44,10 @@ contract PullPayment { /** * @dev Same as {withdrawPayments}, but forwarding all gas to the recipient. * + * 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 here.] + * * 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}. diff --git a/contracts/payment/escrow/Escrow.sol b/contracts/payment/escrow/Escrow.sol index d62948affd9..3a62013903d 100644 --- a/contracts/payment/escrow/Escrow.sol +++ b/contracts/payment/escrow/Escrow.sol @@ -42,8 +42,15 @@ contract Escrow is Secondary { } /** - * @dev Withdraw accumulated balance for a payee. + * @dev Withdraw accumulated balance for a payee, forwarding 2300 gas (a + * Solidity `transfer`). + * * @param payee The address whose funds will be withdrawn and transferred to. + * + * 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.] */ function withdraw(address payable payee) public onlyPrimary { uint256 payment = _deposits[payee]; From 512e5c551e75f96cd64bb489a6846aa4d7d09d56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 28 Oct 2019 18:12:39 -0300 Subject: [PATCH 3/8] Add changelog entry --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b2b85952a20..31eda7d4de5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,11 +11,17 @@ * `Address.toPayable`: added a helper to convert between address types without having to resort to low-level casting. ([#1773](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1773)) * Facilities to make metatransaction-enabled contracts through the Gas Station Network. ([#1844](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1844)) * `Address.sendValue`: added a replacement to Solidity's `transfer`, removing the fixed gas stipend. ([#1961](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1961)) + * `PullPayment.withdrawPaymentsWithGas(address payable payee)`: replacement for the deprecated version (see below), forwarding all available gas. ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) + * `Escrow.withdrawWithGas(address payable payee)`: replacement for the deprecated version (see below), forwarding all available gas. ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) ### Improvements: * `Address.isContract`: switched from `extcodesize` to `extcodehash` for less gas usage. ([#1802](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1802)) * `SafeMath`: added custom error messages support for `sub`, `div` and `mod` functions. `ERC20` and `ERC777` updated to throw custom errors on subtraction overflows. ([#1828](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1828)) +### Deprecations: + * `PullPayment.withdrawPayments(address payable payee)`: due to the payee being forwarded a fixed gas allowance (2300 gas). ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) + * `Escrow.withdraw(address payable payee)`: due to the payee being forwarded a fixed gas allowance (2300 gas). ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) + ### Breaking changes in drafts: * `SignatureBouncer` has been removed from the library, both to avoid confusions with the GSN Bouncers and `GSNBouncerSignature` and because the API was not very clear. ([#1879](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1879)) From 4f59a48a9cf4f0d34ba9f736baf75ef1c2574d8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 28 Oct 2019 19:29:01 -0300 Subject: [PATCH 4/8] Update contracts/payment/PullPayment.sol Co-Authored-By: Francisco Giordano --- contracts/payment/PullPayment.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 3628f877a4e..d8cfcca5c1e 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -33,7 +33,7 @@ contract PullPayment { * {withdrawPayments}. * * NOTE: This function has been deprecated, use {withdrawPaymentsWithGas} - * instead. Calling contracts with fixed-gas limits is an anti-pattern and + * 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.] */ From 96971128aa97e5cdb05a3f6dc02089ec8a6f81f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 28 Oct 2019 19:30:38 -0300 Subject: [PATCH 5/8] Remove repeated comment --- contracts/payment/PullPayment.sol | 4 ---- 1 file changed, 4 deletions(-) diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index d8cfcca5c1e..8fd0ee08317 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -44,10 +44,6 @@ contract PullPayment { /** * @dev Same as {withdrawPayments}, but forwarding all gas to the recipient. * - * 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 here.] - * * 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}. From e0f4d2b4e0709b4316dcd44397ed4cacb5943198 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 28 Oct 2019 19:50:53 -0300 Subject: [PATCH 6/8] Update changelog entry --- CHANGELOG.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6007b49970b..4c9d5e74e20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,16 +11,18 @@ * `Address.toPayable`: added a helper to convert between address types without having to resort to low-level casting. ([#1773](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1773)) * Facilities to make metatransaction-enabled contracts through the Gas Station Network. ([#1844](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1844)) * `Address.sendValue`: added a replacement to Solidity's `transfer`, removing the fixed gas stipend. ([#1962](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1962)) - * `PullPayment.withdrawPaymentsWithGas(address payable payee)`: replacement for the deprecated version (see below), forwarding all available gas. ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) - * `Escrow.withdrawWithGas(address payable payee)`: replacement for the deprecated version (see below), forwarding all available gas. ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) + * Added replacement for functions that don't forward all gas (which have been deprecated): ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) + ** `PullPayment.withdrawPaymentsWithGas(address payable payee)` + ** `Escrow.withdrawWithGas(address payable payee)` ### Improvements: * `Address.isContract`: switched from `extcodesize` to `extcodehash` for less gas usage. ([#1802](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1802)) * `SafeMath`: added custom error messages support for `sub`, `div` and `mod` functions. `ERC20` and `ERC777` updated to throw custom errors on subtraction overflows. ([#1828](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1828)) ### Deprecations: - * `PullPayment.withdrawPayments(address payable payee)`: due to the payee being forwarded a fixed gas allowance (2300 gas). ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) - * `Escrow.withdraw(address payable payee)`: due to the payee being forwarded a fixed gas allowance (2300 gas). ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) + * Deprecated functions that don't forward all gas: ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) + ** `PullPayment.withdrawPayments(address payable payee)` + ** `Escrow.withdraw(address payable payee)` ### Breaking changes in drafts: * `SignatureBouncer` has been removed from the library, both to avoid confusions with the GSN Bouncers and `GSNBouncerSignature` and because the API was not very clear. ([#1879](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1879)) From 110acee6bd62c58342b4fa512a21edb4fd00b2a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 28 Oct 2019 20:01:06 -0300 Subject: [PATCH 7/8] Fix inline docs --- contracts/payment/PullPayment.sol | 9 +++++---- contracts/payment/escrow/Escrow.sol | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 8fd0ee08317..546e7e817f8 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -25,7 +25,6 @@ contract PullPayment { /** * @dev Withdraw accumulated payments. - * @param payee Whose payments will be withdrawn. * * Note that _any_ account can call this function, not just the `payee`. * This means that contracts unaware of the `PullPayment` protocol can still @@ -36,6 +35,8 @@ contract PullPayment { * 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 { _escrow.withdraw(payee); @@ -62,11 +63,11 @@ contract PullPayment { /** * @dev Called by the payer to store the sent amount as credit to be pulled. - * @param dest The destination address of the funds. - * @param amount The amount to transfer. - * * Funds sent in this way are stored in an intermediate {Escrow} contract, so * there is no danger of them being spent before withdrawal. + * + * @param dest The destination address of the funds. + * @param amount The amount to transfer. */ function _asyncTransfer(address dest, uint256 amount) internal { _escrow.deposit.value(amount)(dest); diff --git a/contracts/payment/escrow/Escrow.sol b/contracts/payment/escrow/Escrow.sol index 3a62013903d..7f6b721fe54 100644 --- a/contracts/payment/escrow/Escrow.sol +++ b/contracts/payment/escrow/Escrow.sol @@ -45,12 +45,12 @@ contract Escrow is Secondary { * @dev Withdraw accumulated balance for a payee, forwarding 2300 gas (a * Solidity `transfer`). * - * @param payee The address whose funds will be withdrawn and transferred to. - * * 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 onlyPrimary { uint256 payment = _deposits[payee]; From 7fcb2a8b1f97d7747fd34ff22e2416a24150143a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 28 Oct 2019 20:01:31 -0300 Subject: [PATCH 8/8] Fix changelog formatting --- CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c9d5e74e20..d9d5c26670e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,8 @@ * Facilities to make metatransaction-enabled contracts through the Gas Station Network. ([#1844](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1844)) * `Address.sendValue`: added a replacement to Solidity's `transfer`, removing the fixed gas stipend. ([#1962](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1962)) * Added replacement for functions that don't forward all gas (which have been deprecated): ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) - ** `PullPayment.withdrawPaymentsWithGas(address payable payee)` - ** `Escrow.withdrawWithGas(address payable payee)` + * `PullPayment.withdrawPaymentsWithGas(address payable payee)` + * `Escrow.withdrawWithGas(address payable payee)` ### Improvements: * `Address.isContract`: switched from `extcodesize` to `extcodehash` for less gas usage. ([#1802](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1802)) @@ -21,8 +21,8 @@ ### Deprecations: * Deprecated functions that don't forward all gas: ([#1976](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1976)) - ** `PullPayment.withdrawPayments(address payable payee)` - ** `Escrow.withdraw(address payable payee)` + * `PullPayment.withdrawPayments(address payable payee)` + * `Escrow.withdraw(address payable payee)` ### Breaking changes in drafts: * `SignatureBouncer` has been removed from the library, both to avoid confusions with the GSN Bouncers and `GSNBouncerSignature` and because the API was not very clear. ([#1879](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1879))