Forward all gas on PullPayment withdrawal (#1976)

* Add withdrawWithGas * Improve docs * Add changelog entry * Update contracts/payment/PullPayment.sol Co-Authored-By: Francisco Giordano <[email protected]> * Remove repeated comment * Update changelog entry * Fix inline docs * Fix changelog formatting (cherry picked from commit d6e10ab)
OpenZeppelin · Oct 29, 2019 · 1c220e1 · 1c220e1
1 parent ab4d43c
commit 1c220e1
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,11 +6,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
@@ -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
@@ -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
@@ -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');
+  });
 });