Disallow ERC20._transfer from the zero address. (#1752)

* Add requirement of non-zero from to ERC20 transfer.

* Add test for transferFrom zero address to behavior.

* Create ERC20.transfer behavior.

* Add tests for _transfer.

* Add changelog entry.

* Fix linter error.

* Delete repeated test.

* Fix hardcoded error prefix.

* Update CHANGELOG.md

Co-Authored-By: Francisco Giordano <frangio.1@gmail.com>

* Address review comments.

(cherry picked from commit ad18098d65bbb5e41dcf1a286b8550470039d056)

CHANGELOG.md

@@ -12,6 +12,7 @@
 
 ### Bugfixes:
  * `PostDeliveryCrowdsale`: some validations where skipped when paired with other crowdsale flavors, such as `AllowanceCrowdsale`, or `MintableCrowdsale` and `ERC20Capped`, which could cause buyers to not be able to claim their purchased tokens. ([#1721](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1721))
+ * `ERC20._transfer`: the `from` argument was allowed to be the zero address, so it was possible to internally trigger a transfer of 0 tokens from the zero address. This address is not a valid destinatary of transfers, nor can it give or receive allowance, so this behavior was inconsistent. It now reverts. ([#1752](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1752))
 
 ## 2.2.0 (2019-03-14)
 

contracts/mocks/ERC20Mock.sol

@@ -20,6 +20,10 @@ contract ERC20Mock is ERC20 {
         _burnFrom(account, amount);
     }
 
+    function transferInternal(address from, address to, uint256 value) public {
+        _transfer(from, to, value);
+    }
+
     function approveInternal(address owner, address spender, uint256 value) public {
         _approve(owner, spender, value);
     }

contracts/token/ERC20/ERC20.sol

@@ -125,6 +125,7 @@ contract ERC20 is IERC20 {
      * @param value The amount to be transferred.
      */
     function _transfer(address from, address to, uint256 value) internal {
+        require(from != address(0), "ERC20: transfer from the zero address");
         require(to != address(0), "ERC20: transfer to the zero address");
 
         _balances[from] = _balances[from].sub(value);

test/token/ERC20/ERC20.behavior.js

@@ -23,151 +23,127 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip
   });
 
   describe('transfer', function () {
-    describe('when the recipient is not the zero address', function () {
-      const to = recipient;
-
-      describe('when the sender does not have enough balance', function () {
-        const amount = initialSupply.addn(1);
-
-        it('reverts', async function () {
-          await shouldFail.reverting.withMessage(this.token.transfer(to, amount, { from: initialHolder }),
-            'SafeMath: subtraction overflow'
-          );
-        });
-      });
-
-      describe('when the sender has enough balance', function () {
-        const amount = initialSupply;
-
-        it('transfers the requested amount', async function () {
-          await this.token.transfer(to, amount, { from: initialHolder });
+    shouldBehaveLikeERC20Transfer(errorPrefix, initialHolder, recipient, initialSupply,
+      function (from, to, value) {
+        return this.token.transfer(to, value, { from });
+      }
+    );
+  });
 
-          (await this.token.balanceOf(initialHolder)).should.be.bignumber.equal('0');
+  describe('transfer from', function () {
+    const spender = recipient;
 
-          (await this.token.balanceOf(to)).should.be.bignumber.equal(amount);
-        });
+    describe('when the token owner is not the zero address', function () {
+      const tokenOwner = initialHolder;
 
-        it('emits a transfer event', async function () {
-          const { logs } = await this.token.transfer(to, amount, { from: initialHolder });
+      describe('when the recipient is not the zero address', function () {
+        const to = anotherAccount;
 
-          expectEvent.inLogs(logs, 'Transfer', {
-            from: initialHolder,
-            to: to,
-            value: amount,
+        describe('when the spender has enough approved balance', function () {
+          beforeEach(async function () {
+            await this.token.approve(spender, initialSupply, { from: initialHolder });
           });
-        });
-      });
-    });
-
-    describe('when the recipient is the zero address', function () {
-      const to = ZERO_ADDRESS;
 
-      it('reverts', async function () {
-        await shouldFail.reverting.withMessage(this.token.transfer(to, initialSupply, { from: initialHolder }),
-          `${errorPrefix}: transfer to the zero address`
-        );
-      });
-    });
-  });
+          describe('when the token owner has enough balance', function () {
+            const amount = initialSupply;
 
-  describe('transfer from', function () {
-    const spender = recipient;
+            it('transfers the requested amount', async function () {
+              await this.token.transferFrom(tokenOwner, to, amount, { from: spender });
 
-    describe('when the recipient is not the zero address', function () {
-      const to = anotherAccount;
+              (await this.token.balanceOf(tokenOwner)).should.be.bignumber.equal('0');
 
-      describe('when the spender has enough approved balance', function () {
-        beforeEach(async function () {
-          await this.token.approve(spender, initialSupply, { from: initialHolder });
-        });
+              (await this.token.balanceOf(to)).should.be.bignumber.equal(amount);
+            });
 
-        describe('when the initial holder has enough balance', function () {
-          const amount = initialSupply;
+            it('decreases the spender allowance', async function () {
+              await this.token.transferFrom(tokenOwner, to, amount, { from: spender });
 
-          it('transfers the requested amount', async function () {
-            await this.token.transferFrom(initialHolder, to, amount, { from: spender });
+              (await this.token.allowance(tokenOwner, spender)).should.be.bignumber.equal('0');
+            });
 
-            (await this.token.balanceOf(initialHolder)).should.be.bignumber.equal('0');
+            it('emits a transfer event', async function () {
+              const { logs } = await this.token.transferFrom(tokenOwner, to, amount, { from: spender });
 
-            (await this.token.balanceOf(to)).should.be.bignumber.equal(amount);
-          });
+              expectEvent.inLogs(logs, 'Transfer', {
+                from: tokenOwner,
+                to: to,
+                value: amount,
+              });
+            });
 
-          it('decreases the spender allowance', async function () {
-            await this.token.transferFrom(initialHolder, to, amount, { from: spender });
+            it('emits an approval event', async function () {
+              const { logs } = await this.token.transferFrom(tokenOwner, to, amount, { from: spender });
 
-            (await this.token.allowance(initialHolder, spender)).should.be.bignumber.equal('0');
+              expectEvent.inLogs(logs, 'Approval', {
+                owner: tokenOwner,
+                spender: spender,
+                value: await this.token.allowance(tokenOwner, spender),
+              });
+            });
           });
 
-          it('emits a transfer event', async function () {
-            const { logs } = await this.token.transferFrom(initialHolder, to, amount, { from: spender });
+          describe('when the token owner does not have enough balance', function () {
+            const amount = initialSupply.addn(1);
 
-            expectEvent.inLogs(logs, 'Transfer', {
-              from: initialHolder,
-              to: to,
-              value: amount,
+            it('reverts', async function () {
+              await shouldFail.reverting.withMessage(this.token.transferFrom(
+                tokenOwner, to, amount, { from: spender }), 'SafeMath: subtraction overflow'
+              );
             });
           });
+        });
+
+        describe('when the spender does not have enough approved balance', function () {
+          beforeEach(async function () {
+            await this.token.approve(spender, initialSupply.subn(1), { from: tokenOwner });
+          });
 
-          it('emits an approval event', async function () {
-            const { logs } = await this.token.transferFrom(initialHolder, to, amount, { from: spender });
+          describe('when the token owner has enough balance', function () {
+            const amount = initialSupply;
 
-            expectEvent.inLogs(logs, 'Approval', {
-              owner: initialHolder,
-              spender: spender,
-              value: await this.token.allowance(initialHolder, spender),
+            it('reverts', async function () {
+              await shouldFail.reverting.withMessage(this.token.transferFrom(
+                tokenOwner, to, amount, { from: spender }), 'SafeMath: subtraction overflow'
+              );
             });
           });
-        });
 
-        describe('when the initial holder does not have enough balance', function () {
-          const amount = initialSupply.addn(1);
+          describe('when the token owner does not have enough balance', function () {
+            const amount = initialSupply.addn(1);
 
-          it('reverts', async function () {
-            await shouldFail.reverting.withMessage(this.token.transferFrom(
-              initialHolder, to, amount, { from: spender }), 'SafeMath: subtraction overflow'
-            );
+            it('reverts', async function () {
+              await shouldFail.reverting.withMessage(this.token.transferFrom(
+                tokenOwner, to, amount, { from: spender }), 'SafeMath: subtraction overflow'
+              );
+            });
           });
         });
       });
 
-      describe('when the spender does not have enough approved balance', function () {
-        beforeEach(async function () {
-          await this.token.approve(spender, initialSupply.subn(1), { from: initialHolder });
-        });
-
-        describe('when the initial holder has enough balance', function () {
-          const amount = initialSupply;
+      describe('when the recipient is the zero address', function () {
+        const amount = initialSupply;
+        const to = ZERO_ADDRESS;
 
-          it('reverts', async function () {
-            await shouldFail.reverting.withMessage(this.token.transferFrom(
-              initialHolder, to, amount, { from: spender }), 'SafeMath: subtraction overflow'
-            );
-          });
+        beforeEach(async function () {
+          await this.token.approve(spender, amount, { from: tokenOwner });
         });
 
-        describe('when the initial holder does not have enough balance', function () {
-          const amount = initialSupply.addn(1);
-
-          it('reverts', async function () {
-            await shouldFail.reverting.withMessage(this.token.transferFrom(
-              initialHolder, to, amount, { from: spender }), 'SafeMath: subtraction overflow'
-            );
-          });
+        it('reverts', async function () {
+          await shouldFail.reverting.withMessage(this.token.transferFrom(
+            tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer to the zero address`
+          );
         });
       });
     });
 
-    describe('when the recipient is the zero address', function () {
-      const amount = initialSupply;
-      const to = ZERO_ADDRESS;
-
-      beforeEach(async function () {
-        await this.token.approve(spender, amount, { from: initialHolder });
-      });
+    describe('when the token owner is the zero address', function () {
+      const amount = 0;
+      const tokenOwner = ZERO_ADDRESS;
+      const to = recipient;
 
       it('reverts', async function () {
         await shouldFail.reverting.withMessage(this.token.transferFrom(
-          initialHolder, to, amount, { from: spender }), `${errorPrefix}: transfer to the zero address`
+          tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer from the zero address`
         );
       });
     });
@@ -182,6 +158,72 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip
   });
 }
 
+function shouldBehaveLikeERC20Transfer (errorPrefix, from, to, balance, transfer) {
+  describe('when the recipient is not the zero address', function () {
+    describe('when the sender does not have enough balance', function () {
+      const amount = balance.addn(1);
+
+      it('reverts', async function () {
+        await shouldFail.reverting.withMessage(transfer.call(this, from, to, amount),
+          'SafeMath: subtraction overflow'
+        );
+      });
+    });
+
+    describe('when the sender transfers all balance', function () {
+      const amount = balance;
+
+      it('transfers the requested amount', async function () {
+        await transfer.call(this, from, to, amount);
+
+        (await this.token.balanceOf(from)).should.be.bignumber.equal('0');
+
+        (await this.token.balanceOf(to)).should.be.bignumber.equal(amount);
+      });
+
+      it('emits a transfer event', async function () {
+        const { logs } = await transfer.call(this, from, to, amount);
+
+        expectEvent.inLogs(logs, 'Transfer', {
+          from,
+          to,
+          value: amount,
+        });
+      });
+    });
+
+    describe('when the sender transfers zero tokens', function () {
+      const amount = new BN('0');
+
+      it('transfers the requested amount', async function () {
+        await transfer.call(this, from, to, amount);
+
+        (await this.token.balanceOf(from)).should.be.bignumber.equal(balance);
+
+        (await this.token.balanceOf(to)).should.be.bignumber.equal('0');
+      });
+
+      it('emits a transfer event', async function () {
+        const { logs } = await transfer.call(this, from, to, amount);
+
+        expectEvent.inLogs(logs, 'Transfer', {
+          from,
+          to,
+          value: amount,
+        });
+      });
+    });
+  });
+
+  describe('when the recipient is the zero address', function () {
+    it('reverts', async function () {
+      await shouldFail.reverting.withMessage(transfer.call(this, from, ZERO_ADDRESS, balance),
+        `${errorPrefix}: transfer to the zero address`
+      );
+    });
+  });
+}
+
 function shouldBehaveLikeERC20Approve (errorPrefix, owner, spender, supply, approve) {
   describe('when the spender is not the zero address', function () {
     describe('when the sender has enough balance', function () {
@@ -264,5 +306,6 @@ function shouldBehaveLikeERC20Approve (errorPrefix, owner, spender, supply, appr
 
 module.exports = {
   shouldBehaveLikeERC20,
+  shouldBehaveLikeERC20Transfer,
   shouldBehaveLikeERC20Approve,
 };

test/token/ERC20/ERC20.test.js

@@ -3,6 +3,7 @@ const { ZERO_ADDRESS } = constants;
 
 const {
   shouldBehaveLikeERC20,
+  shouldBehaveLikeERC20Transfer,
   shouldBehaveLikeERC20Approve,
 } = require('./ERC20.behavior');
 
@@ -330,6 +331,20 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) {
     });
   });
 
+  describe('_transfer', function () {
+    shouldBehaveLikeERC20Transfer('ERC20', initialHolder, recipient, initialSupply, function (from, to, amount) {
+      return this.token.transferInternal(from, to, amount);
+    });
+
+    describe('when the sender is the zero address', function () {
+      it('reverts', async function () {
+        await shouldFail.reverting.withMessage(this.token.transferInternal(ZERO_ADDRESS, recipient, initialSupply),
+          'ERC20: transfer from the zero address'
+        );
+      });
+    });
+  });
+
   describe('_approve', function () {
     shouldBehaveLikeERC20Approve('ERC20', initialHolder, recipient, initialSupply, function (owner, spender, amount) {
       return this.token.approveInternal(owner, spender, amount);