diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ffc3dbd4bb..b2b85952a20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,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 7697fdad42e..52563608f28 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 00000000000..a2355b80382 --- /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 dee891b6d09..c9e7711b355 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 a13123598de..7013ed13a6e 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' + ); + }); + }); + }); + }); });