diff --git a/contracts/token/ERC20/SafeERC20.sol b/contracts/token/ERC20/SafeERC20.sol index c7c6efb22..e632c27f5 100644 --- a/contracts/token/ERC20/SafeERC20.sol +++ b/contracts/token/ERC20/SafeERC20.sol @@ -2,6 +2,7 @@ pragma solidity ^0.5.2; import "./IERC20.sol"; import "../../math/SafeMath.sol"; +import "../../utils/Address.sol"; /** * @title SafeERC20 @@ -14,6 +15,7 @@ import "../../math/SafeMath.sol"; */ library SafeERC20 { using SafeMath for uint256; + using Address for address; function safeTransfer(IERC20 token, address to, uint256 value) internal { callOptionalReturn(token, abi.encodeWithSelector(token.transfer.selector, to, value)); @@ -51,11 +53,18 @@ library SafeERC20 { // We need to perform a low level call here, to bypass Solidity's return data size checking mechanism, since // we're implementing it ourselves. + // A Solidity high level call has three parts: + // 1. The target address is checked to verify it contains contract code + // 2. The call itself is made, and success asserted + // 3. The return value is decoded, which in turn checks the size of the returned data. + + require(address(token).isContract()); + // solhint-disable-next-line avoid-low-level-calls (bool success, bytes memory returndata) = address(token).call(data); require(success); - if (returndata.length > 0) { + if (returndata.length > 0) { // Return data is optional require(abi.decode(returndata, (bool))); } } diff --git a/test/token/ERC20/SafeERC20.test.js b/test/token/ERC20/SafeERC20.test.js index 9acc3a446..fee236801 100644 --- a/test/token/ERC20/SafeERC20.test.js +++ b/test/token/ERC20/SafeERC20.test.js @@ -5,31 +5,21 @@ const ERC20ReturnTrueMock = artifacts.require('ERC20ReturnTrueMock'); const ERC20NoReturnMock = artifacts.require('ERC20NoReturnMock'); const SafeERC20Wrapper = artifacts.require('SafeERC20Wrapper'); -contract('SafeERC20', function () { +contract('SafeERC20', function ([_, hasNoCode]) { + describe('with address that has no contract code', function () { + beforeEach(async function () { + this.wrapper = await SafeERC20Wrapper.new(hasNoCode); + }); + + shouldRevertOnAllCalls(); + }); + describe('with token that returns false on all calls', function () { beforeEach(async function () { this.wrapper = await SafeERC20Wrapper.new((await ERC20ReturnFalseMock.new()).address); }); - it('reverts on transfer', async function () { - await shouldFail.reverting(this.wrapper.transfer()); - }); - - it('reverts on transferFrom', async function () { - await shouldFail.reverting(this.wrapper.transferFrom()); - }); - - it('reverts on approve', async function () { - await shouldFail.reverting(this.wrapper.approve(0)); - }); - - it('reverts on increaseAllowance', async function () { - await shouldFail.reverting(this.wrapper.increaseAllowance(0)); - }); - - it('reverts on decreaseAllowance', async function () { - await shouldFail.reverting(this.wrapper.decreaseAllowance(0)); - }); + shouldRevertOnAllCalls(); }); describe('with token that returns true on all calls', function () { @@ -49,6 +39,28 @@ contract('SafeERC20', function () { }); }); +function shouldRevertOnAllCalls () { + it('reverts on transfer', async function () { + await shouldFail.reverting(this.wrapper.transfer()); + }); + + it('reverts on transferFrom', async function () { + await shouldFail.reverting(this.wrapper.transferFrom()); + }); + + it('reverts on approve', async function () { + await shouldFail.reverting(this.wrapper.approve(0)); + }); + + it('reverts on increaseAllowance', async function () { + await shouldFail.reverting(this.wrapper.increaseAllowance(0)); + }); + + it('reverts on decreaseAllowance', async function () { + await shouldFail.reverting(this.wrapper.decreaseAllowance(0)); + }); +} + function shouldOnlyRevertOnErrors () { it('doesn\'t revert on transfer', async function () { await this.wrapper.transfer();