Forward all gas on PullPayment withdrawal

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. ([#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))
 
 ### 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))
 

contracts/payment/PullPayment.sol

@@ -31,11 +31,31 @@ 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);
     }
 
+    /**
+     * @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}.
+     */
+    function withdrawPaymentsWithGas(address payable payee) public {
+        _escrow.withdrawWithGas(payee);
+    }
+
     /**
      * @dev Returns the payments owed to an address.
      * @param dest The creditor's address.

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,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];
@@ -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);
+    }
 }

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');
+  });
 });