From d6e10ab78625834861dd16489135064a745e1941 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 28 Oct 2019 20:06:02 -0300 Subject: [PATCH] Forward all gas on PullPayment withdrawal (#1976) * Add withdrawWithGas * Improve docs * Add changelog entry * Update contracts/payment/PullPayment.sol Co-Authored-By: Francisco Giordano * Remove repeated comment * Update changelog entry * Fix inline docs * Fix changelog formatting --- CHANGELOG.md | 8 ++++++++ contracts/payment/PullPayment.sol | 25 +++++++++++++++++++++---- contracts/payment/escrow/Escrow.sol | 28 +++++++++++++++++++++++++++- test/payment/PullPayment.test.js | 12 ++++++++++++ 4 files changed, 68 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa8ab2d593d..d9d5c26670e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,11 +11,19 @@ * `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)) + * 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: + * 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)) diff --git a/contracts/payment/PullPayment.sol b/contracts/payment/PullPayment.sol index 594d618f2dd..546e7e817f8 100644 --- a/contracts/payment/PullPayment.sol +++ b/contracts/payment/PullPayment.sol @@ -25,17 +25,34 @@ 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 * 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 { _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. @@ -46,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 4422d328e87..7f6b721fe54 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); @@ -40,7 +42,14 @@ contract Escrow is Secondary { } /** - * @dev Withdraw accumulated balance for a payee. + * @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 onlyPrimary { @@ -52,4 +61,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'); + }); });