Add extcodesize check to SafeERC20. (#1662)

* Add extcodesize check to SafeERC20.

* Clarify some comments.

* Replace inline assembly with Address.sol.

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)));
         }
     }

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 () {
-  describe('with token that returns false on all calls', function () {
+contract('SafeERC20', function ([_, hasNoCode]) {
+  describe('with address that has no contract code', 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());
+      this.wrapper = await SafeERC20Wrapper.new(hasNoCode);
     });
 
-    it('reverts on approve', async function () {
-      await shouldFail.reverting(this.wrapper.approve(0));
-    });
+    shouldRevertOnAllCalls();
+  });
 
-    it('reverts on increaseAllowance', async function () {
-      await shouldFail.reverting(this.wrapper.increaseAllowance(0));
+  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 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();