Transfer replacement (#1962)

* Add Address.sendEther

* Add documentation to sendEther

* Add changelog entry

* Rename sendEther to sendValue

(cherry picked from commit 8d166f3e35)

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### New features:
  * `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))
 
 ### Improvements:
  * `Address.isContract`: switched from `extcodesize` to `extcodehash` for less gas usage. ([#1802](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1802))

contracts/mocks/AddressImpl.sol

@@ -10,4 +10,10 @@ contract AddressImpl {
     function toPayable(address account) external pure returns (address payable) {
         return Address.toPayable(account);
     }
+
+    function sendValue(address payable receiver, uint256 amount) external {
+        Address.sendValue(receiver, amount);
+    }
+
+    function () external payable { } // sendValue's tests require the contract to hold Ether
 }

contracts/mocks/EtherReceiverMock.sol

@@ -0,0 +1,15 @@
+pragma solidity ^0.5.0;
+
+contract EtherReceiverMock {
+    bool private _acceptEther;
+
+    function setAcceptEther(bool acceptEther) public {
+        _acceptEther = acceptEther;
+    }
+
+    function () external payable {
+        if (!_acceptEther) {
+            revert();
+        }
+    }
+}

contracts/utils/Address.sol

@@ -40,4 +40,28 @@ library Address {
     function toPayable(address account) internal pure returns (address payable) {
         return address(uint160(account));
     }
+
+    /**
+     * @dev Replacement for Solidity's `transfer`: sends `amount` wei to
+     * `recipient`, forwarding all available gas and reverting on errors.
+     *
+     * https://eips.ethereum.org/EIPS/eip-1884[EIP1884] increases the gas cost
+     * of certain opcodes, possibly making contracts go over the 2300 gas limit
+     * imposed by `transfer`, making them unable to receive funds via
+     * `transfer`. {sendValue} removes this limitation.
+     *
+     * https://diligence.consensys.net/posts/2019/09/stop-using-soliditys-transfer-now/[Learn more].
+     *
+     * IMPORTANT: because control is transferred to `recipient`, care must be
+     * taken to not create reentrancy vulnerabilities. Consider using
+     * {ReentrancyGuard} or the
+     * https://solidity.readthedocs.io/en/v0.5.11/security-considerations.html#use-the-checks-effects-interactions-pattern[checks-effects-interactions pattern].
+     */
+    function sendValue(address payable recipient, uint256 amount) internal {
+        require(address(this).balance >= amount, "Address: insufficient balance");
+
+        // solhint-disable-next-line avoid-call-value
+        (bool success, ) = recipient.call.value(amount)("");
+        require(success, "Address: unable to send value, recipient may have reverted");
+    }
 }

test/utils/Address.test.js

@@ -1,10 +1,11 @@
-const { constants } = require('openzeppelin-test-helpers');
+const { balance, constants, ether, expectRevert, send } = require('openzeppelin-test-helpers');
 const { expect } = require('chai');
 
 const AddressImpl = artifacts.require('AddressImpl');
 const SimpleToken = artifacts.require('SimpleToken');
+const EtherReceiver = artifacts.require('EtherReceiverMock');
 
-contract('Address', function ([_, other]) {
+contract('Address', function ([_, recipient, other]) {
   const ALL_ONES_ADDRESS = '0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF';
 
   beforeEach(async function () {
@@ -35,4 +36,71 @@ contract('Address', function ([_, other]) {
       expect(await this.mock.toPayable(ALL_ONES_ADDRESS)).to.equal(ALL_ONES_ADDRESS);
     });
   });
+
+  describe('sendValue', function () {
+    beforeEach(async function () {
+      this.recipientTracker = await balance.tracker(recipient);
+    });
+
+    context('when sender contract has no funds', function () {
+      it('sends 0 wei', async function () {
+        await this.mock.sendValue(other, 0);
+
+        expect(await this.recipientTracker.delta()).to.be.bignumber.equal('0');
+      });
+
+      it('reverts when sending non-zero amounts', async function () {
+        await expectRevert(this.mock.sendValue(other, 1), 'Address: insufficient balance');
+      });
+    });
+
+    context('when sender contract has funds', function () {
+      const funds = ether('1');
+      beforeEach(async function () {
+        await send.ether(other, this.mock.address, funds);
+      });
+
+      it('sends 0 wei', async function () {
+        await this.mock.sendValue(recipient, 0);
+        expect(await this.recipientTracker.delta()).to.be.bignumber.equal('0');
+      });
+
+      it('sends non-zero amounts', async function () {
+        await this.mock.sendValue(recipient, funds.subn(1));
+        expect(await this.recipientTracker.delta()).to.be.bignumber.equal(funds.subn(1));
+      });
+
+      it('sends the whole balance', async function () {
+        await this.mock.sendValue(recipient, funds);
+        expect(await this.recipientTracker.delta()).to.be.bignumber.equal(funds);
+        expect(await balance.current(this.mock.address)).to.be.bignumber.equal('0');
+      });
+
+      it('reverts when sending more than the balance', async function () {
+        await expectRevert(this.mock.sendValue(recipient, funds.addn(1)), 'Address: insufficient balance');
+      });
+
+      context('with contract recipient', function () {
+        beforeEach(async function () {
+          this.contractRecipient = await EtherReceiver.new();
+        });
+
+        it('sends funds', async function () {
+          const tracker = await balance.tracker(this.contractRecipient.address);
+
+          await this.contractRecipient.setAcceptEther(true);
+          await this.mock.sendValue(this.contractRecipient.address, funds);
+          expect(await tracker.delta()).to.be.bignumber.equal(funds);
+        });
+
+        it('reverts on recipient revert', async function () {
+          await this.contractRecipient.setAcceptEther(false);
+          await expectRevert(
+            this.mock.sendValue(this.contractRecipient.address, funds),
+            'Address: unable to send value, recipient may have reverted'
+          );
+        });
+      });
+    });
+  });
 });