Add missing requirements to ERC777 (#2212)

* Restore _approve check for zero sender * Add non-zero operator check to _send
OpenZeppelin · Apr 24, 2020 · c75b016 · c75b016
1 parent 58a3368
commit c75b016
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 5 deletions.
diff --git a/contracts/mocks/ERC777Mock.sol b/contracts/mocks/ERC777Mock.sol
@@ -23,4 +23,8 @@ contract ERC777Mock is Context, ERC777 {
     ) public {
         _mint(operator, to, amount, userData, operatorData);
     }
+
+    function approveInternal(address holder, address spender, uint256 value) public {
+        _approve(holder, spender, value);
+    }
 }
diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol
@@ -352,6 +352,7 @@ contract ERC777 is Context, IERC777, IERC20 {
     )
         internal
     {
+        require(operator != address(0), "ERC777: operator is the zero address");
         require(from != address(0), "ERC777: send from the zero address");
         require(to != address(0), "ERC777: send to the zero address");
 
@@ -408,10 +409,13 @@ contract ERC777 is Context, IERC777, IERC20 {
         emit Transfer(from, to, amount);
     }
 
+    /**
+     * @dev See {ERC20-_approve}.
+     *
+     * Note that accounts cannot have allowance issued by their operators.
+     */
     function _approve(address holder, address spender, uint256 value) internal {
-        // TODO: restore this require statement if this function becomes internal, or is called at a new callsite. It is
-        // currently unnecessary.
-        //require(holder != address(0), "ERC777: approve from the zero address");
+        require(holder != address(0), "ERC777: approve from the zero address");
         require(spender != address(0), "ERC777: approve to the zero address");
 
         _allowances[holder][spender] = value;

diff --git a/test/token/ERC777/ERC777.test.js b/test/token/ERC777/ERC777.test.js
@@ -1,6 +1,7 @@
 const { accounts, contract, web3 } = require('@openzeppelin/test-environment');
 
-const { BN, expectEvent, expectRevert, singletons } = require('@openzeppelin/test-helpers');
+const { BN, constants, expectEvent, expectRevert, singletons } = require('@openzeppelin/test-helpers');
+const { ZERO_ADDRESS } = constants;
 
 const { expect } = require('chai');
 
@@ -15,6 +16,7 @@ const {
 
 const {
   shouldBehaveLikeERC20,
+  shouldBehaveLikeERC20Approve,
 } = require('../ERC20/ERC20.behavior');
 
 const ERC777 = contract.fromArtifact('ERC777Mock');
@@ -40,7 +42,23 @@ describe('ERC777', function () {
       this.token = await ERC777.new(holder, initialSupply, name, symbol, defaultOperators);
     });
 
-    shouldBehaveLikeERC20('ERC777', initialSupply, holder, anyone, defaultOperatorA);
+    describe('as an ERC20 token', function () {
+      shouldBehaveLikeERC20('ERC777', initialSupply, holder, anyone, defaultOperatorA);
+
+      describe('_approve', function () {
+        shouldBehaveLikeERC20Approve('ERC777', holder, anyone, initialSupply, function (owner, spender, amount) {
+          return this.token.approveInternal(owner, spender, amount);
+        });
+
+        describe('when the owner is the zero address', function () {
+          it('reverts', async function () {
+            await expectRevert(this.token.approveInternal(ZERO_ADDRESS, anyone, initialSupply),
+              'ERC777: approve from the zero address'
+            );
+          });
+        });
+      });
+    });
 
     it.skip('does not emit AuthorizedOperator events for default operators', async function () {
       expectEvent.not.inConstructor(this.token, 'AuthorizedOperator'); // This helper needs to be implemented