From b0dbe0fc59181994a0f94f2bd8bb8d7b9c730749 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Fri, 25 Oct 2019 15:53:16 -0300 Subject: [PATCH] Transfer replacement (#1962) * Add Address.sendEther * Add documentation to sendEther * Add changelog entry * Rename sendEther to sendValue (cherry picked from commit 8d166f3e35d3021e4ac9b23f1ec54bc2a148d880) --- CHANGELOG.md | 1 + contracts/mocks/AddressImpl.sol | 6 +++ contracts/mocks/EtherReceiverMock.sol | 15 ++++++ contracts/utils/Address.sol | 24 +++++++++ test/utils/Address.test.js | 72 ++++++++++++++++++++++++++- 5 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 contracts/mocks/EtherReceiverMock.sol diff --git a/CHANGELOG.md b/CHANGELOG.md index ac73a954e..bfdc9e2fe 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/contracts/mocks/AddressImpl.sol b/contracts/mocks/AddressImpl.sol index 7697fdad4..52563608f 100644 --- a/contracts/mocks/AddressImpl.sol +++ b/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 } diff --git a/contracts/mocks/EtherReceiverMock.sol b/contracts/mocks/EtherReceiverMock.sol new file mode 100644 index 000000000..a2355b803 --- /dev/null +++ b/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(); + } + } +} diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index dee891b6d..c9e7711b3 100644 --- a/contracts/utils/Address.sol +++ b/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"); + } } diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js index 78853b528..7c25b68d1 100644 --- a/test/utils/Address.test.js +++ b/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' + ); + }); + }); + }); + }); });