Merge pull request #23 from nventuro/oz-sol-rc3

Merging OZ 2.0.0.rc3

.github/PULL_REQUEST_TEMPLATE.md

@@ -14,4 +14,4 @@ Fixes #
 - [ ] 📘 I've reviewed the [OpenZeppelin Contributor Guidelines](../blob/master/CONTRIBUTING.md)
 - [ ] ✅ I've added tests where applicable to test my new functionality.
 - [ ] 📖 I've made sure that my contracts are well-documented.
-- [ ] 🎨 I've run the JS/Solidity linters and fixed any issues (`npm run lint:all:fix`).
+- [ ] 🎨 I've run the JS/Solidity linters and fixed any issues (`npm run lint:fix`).

contracts/drafts/BreakInvariantBounty.sol

@@ -53,6 +53,7 @@ contract BreakInvariantBounty is Initializable, PullPayment, Ownable {
    * @param target contract
    */
   function claim(Target target) public {
+    require(!_claimed);
     address researcher = _researchers[target];
     require(researcher != address(0));
     // Check Target contract invariants

contracts/mocks/ERC721FullMock.sol

@@ -3,15 +3,16 @@ pragma solidity ^0.4.24;
 import "../Initializable.sol";
 import "../token/ERC721/ERC721Full.sol";
 import "../token/ERC721/ERC721Mintable.sol";
+import "../token/ERC721/ERC721MetadataMintable.sol";
 import "../token/ERC721/ERC721Burnable.sol";
 
 
 /**
- * @title ERC721Mock
+ * @title ERC721FullMock
  * This mock just provides a public mint and burn functions for testing purposes,
  * and a public setter for metadata URI
  */
-contract ERC721FullMock is Initializable, ERC721Full, ERC721Mintable, ERC721Burnable {
+contract ERC721FullMock is Initializable, ERC721Full, ERC721Mintable, ERC721MetadataMintable, ERC721Burnable {
   constructor(string name, string symbol) public
   {
     ERC721Full.initialize(name, symbol);

contracts/mocks/ERC721MintableBurnableImpl.sol

@@ -3,6 +3,7 @@ pragma solidity ^0.4.24;
 import "../Initializable.sol";
 import "../token/ERC721/ERC721Full.sol";
 import "../token/ERC721/ERC721Mintable.sol";
+import "../token/ERC721/ERC721MetadataMintable.sol";
 import "../token/ERC721/ERC721Burnable.sol";
 
 
@@ -10,7 +11,7 @@ import "../token/ERC721/ERC721Burnable.sol";
  * @title ERC721MintableBurnableImpl
  */
 contract ERC721MintableBurnableImpl
-  is Initializable, ERC721Full, ERC721Mintable, ERC721Burnable {
+  is Initializable, ERC721Full, ERC721Mintable, ERC721MetadataMintable, ERC721Burnable {
 
   constructor()
     public

contracts/mocks/SignatureBouncerMock.sol

@@ -69,4 +69,11 @@ contract SignatureBouncerMock is Initializable, SignatureBouncer, SignerRoleMock
   {
 
   }
+
+  function tooShortMsgData()
+    public
+    onlyValidSignatureAndData("")
+    view
+  {
+  }
 }

contracts/payment/RefundEscrow.sol

@@ -2,7 +2,6 @@ pragma solidity ^0.4.24;
 
 import "../Initializable.sol";
 import "./ConditionalEscrow.sol";
-import "../ownership/Secondary.sol";
 
 
 /**
@@ -11,7 +10,7 @@ import "../ownership/Secondary.sol";
  * The primary account may close the deposit period, and allow for either withdrawal
  * by the beneficiary, or refunds to the depositors.
  */
-contract RefundEscrow is Initializable, Secondary, ConditionalEscrow {
+contract RefundEscrow is Initializable, ConditionalEscrow {
   enum State { Active, Refunding, Closed }
 
   event Closed();

contracts/token/ERC20/ERC20.sol

@@ -60,12 +60,7 @@ contract ERC20 is Initializable, IERC20 {
   * @param value The amount to be transferred.
   */
   function transfer(address to, uint256 value) public returns (bool) {
-    require(value <= _balances[msg.sender]);
-    require(to != address(0));
-
-    _balances[msg.sender] = _balances[msg.sender].sub(value);
-    _balances[to] = _balances[to].add(value);
-    emit Transfer(msg.sender, to, value);
+    _transfer(msg.sender, to, value);
     return true;
   }
 
@@ -100,14 +95,10 @@ contract ERC20 is Initializable, IERC20 {
     public
     returns (bool)
   {
-    require(value <= _balances[from]);
     require(value <= _allowed[from][msg.sender]);
-    require(to != address(0));
 
-    _balances[from] = _balances[from].sub(value);
-    _balances[to] = _balances[to].add(value);
     _allowed[from][msg.sender] = _allowed[from][msg.sender].sub(value);
-    emit Transfer(from, to, value);
+    _transfer(from, to, value);
     return true;
   }
 
@@ -160,6 +151,21 @@ contract ERC20 is Initializable, IERC20 {
   }
 
   /**
+  * @dev Transfer token for a specified addresses
+  * @param from The address to transfer from.
+  * @param to The address to transfer to.
+  * @param value The amount to be transferred.
+  */
+  function _transfer(address from, address to, uint256 value) internal {
+    require(value <= _balances[from]);
+    require(to != address(0));
+
+    _balances[from] = _balances[from].sub(value);
+    _balances[to] = _balances[to].add(value);
+    emit Transfer(from, to, value);
+  }
+
+  /**
    * @dev Internal function that mints an amount of the token and assigns it to
    * an account. This encapsulates the modification of balances such that the
    * proper events are emitted.

contracts/token/ERC20/ERC20Burnable.sol

@@ -26,12 +26,4 @@ contract ERC20Burnable is Initializable, ERC20 {
   function burnFrom(address from, uint256 value) public {
     _burnFrom(from, value);
   }
-
-  /**
-   * @dev Overrides ERC20._burn in order for burn and burnFrom to emit
-   * an additional Burn event.
-   */
-  function _burn(address who, uint256 value) internal {
-    super._burn(who, value);
-  }
 }

contracts/token/ERC721/ERC721MetadataMintable.sol

+pragma solidity ^0.4.24;
+
+import "./ERC721Metadata.sol";
+import "../../access/roles/MinterRole.sol";
+
+
+/**
+ * @title ERC721MetadataMintable
+ * @dev ERC721 minting logic with metadata
+ */
+contract ERC721MetadataMintable is ERC721, ERC721Metadata, MinterRole {
+  /**
+   * @dev Function to mint tokens
+   * @param to The address that will receive the minted tokens.
+   * @param tokenId The token id to mint.
+   * @param tokenURI The token URI of the minted token.
+   * @return A boolean that indicates if the operation was successful.
+   */
+  function mintWithTokenURI(
+    address to,
+    uint256 tokenId,
+    string tokenURI
+  )
+    public
+    onlyMinter
+    returns (bool)
+  {
+    _mint(to, tokenId);
+    _setTokenURI(tokenId, tokenURI);
+    return true;
+  }
+}

contracts/token/ERC721/ERC721Mintable.sol

 pragma solidity ^0.4.24;
 
 import "../../Initializable.sol";
-import "./ERC721Full.sol";
+import "./ERC721.sol";
 import "../../access/roles/MinterRole.sol";
 
 
@@ -9,7 +9,7 @@ import "../../access/roles/MinterRole.sol";
  * @title ERC721Mintable
  * @dev ERC721 minting logic
  */
-contract ERC721Mintable is Initializable, ERC721Full, MinterRole {
+contract ERC721Mintable is Initializable, ERC721, MinterRole {
   function initialize() public initializer {
     MinterRole.initialize();
   }
@@ -31,18 +31,4 @@ contract ERC721Mintable is Initializable, ERC721Full, MinterRole {
     _mint(to, tokenId);
     return true;
   }
-
-  function mintWithTokenURI(
-    address to,
-    uint256 tokenId,
-    string tokenURI
-  )
-    public
-    onlyMinter
-    returns (bool)
-  {
-    mint(to, tokenId);
-    _setTokenURI(tokenId, tokenURI);
-    return true;
-  }
 }

ethpm.json

 {
   "package_name": "zeppelin",
-  "version": "2.0.0-rc.2",
+  "version": "2.0.0-rc.3",
   "description": "Secure Smart Contract library for Solidity",
   "authors": [
     "OpenZeppelin Community <maintainers@openzeppelin.org>"

package-lock.json

 {
   "name": "openzeppelin-zos",
-  "version": "2.0.0-rc.1",
+  "version": "2.0.0-rc.3",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {

package.json

 {
   "name": "openzeppelin-zos",
-  "version": "2.0.0-rc.1",
+  "version": "2.0.0-rc.3",
   "description": "Secure Smart Contract library for Solidity",
   "files": [
     "build",

test/drafts/BreakInvariantBounty.test.js

@@ -92,6 +92,10 @@ contract('BreakInvariantBounty', function ([_, owner, researcher, anyone, nonTar
             it('no longer accepts rewards', async function () {
               await assertRevert(ethSendTransaction({ from: owner, to: this.bounty.address, value: reward }));
             });
+
+            it('reverts when reclaimed', async function () {
+              await assertRevert(this.bounty.claim(this.target.address, { from: researcher }));
+            });
           });
         });
       });

test/drafts/SignatureBouncer.test.js

@@ -128,6 +128,12 @@ contract('SignatureBouncer', function ([_, signer, otherSigner, anyone, authoriz
           )
         );
       });
+
+      it('does not allow msg.data shorter than SIGNATURE_SIZE', async function () {
+        await assertRevert(
+          this.sigBouncer.tooShortMsgData({ from: authorizedUser })
+        );
+      });
     });
   });
 

test/library/ECDSA.test.js

@@ -11,56 +11,117 @@ const WRONG_MESSAGE = web3.sha3('Nope');
 
 contract('ECDSA', function ([_, anyone]) {
   beforeEach(async function () {
-    this.mock = await ECDSAMock.new();
+    this.ecdsa = await ECDSAMock.new();
   });
 
-  it('recover v0', async function () {
-    // Signature generated outside ganache with method web3.eth.sign(signer, message)
-    const signer = '0x2cc1166f6212628a0deef2b33befb2187d35b86c';
-    // eslint-disable-next-line max-len
-    const signature = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be89200';
-    (await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
-  });
+  context('recover with valid signature', function () {
+    context('with v0 signature', function () {
+      // Signature generated outside ganache with method web3.eth.sign(signer, message)
+      const signer = '0x2cc1166f6212628a0deef2b33befb2187d35b86c';
+      // eslint-disable-next-line max-len
+      const signatureWithoutVersion = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892';
 
-  it('recover v1', async function () {
-    // Signature generated outside ganache with method web3.eth.sign(signer, message)
-    const signer = '0x1e318623ab09fe6de3c9b8672098464aeda9100e';
-    // eslint-disable-next-line max-len
-    const signature = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e001';
-    (await this.mock.recover(TEST_MESSAGE, signature)).should.equal(signer);
-  });
+      context('with 00 as version value', function () {
+        it('works', async function () {
+          const version = '00';
+          const signature = signatureWithoutVersion + version;
+          (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(signer);
+        });
+      });
 
-  it('recover using web3.eth.sign()', async function () {
-    // Create the signature
-    const signature = signMessage(anyone, TEST_MESSAGE);
+      context('with 27 as version value', function () {
+        it('works', async function () {
+          const version = '1b'; // 27 = 1b.
+          const signature = signatureWithoutVersion + version;
+          (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(signer);
+        });
+      });
 
-    // Recover the signer address from the generated message and signature.
-    (await this.mock.recover(
-      toEthSignedMessageHash(TEST_MESSAGE),
-      signature
-    )).should.equal(anyone);
-  });
+      context('with wrong version', function () {
+        it('returns 0', async function () {
+          // The last two hex digits are the signature version.
+          // The only valid values are 0, 1, 27 and 28.
+          const version = '02';
+          const signature = signatureWithoutVersion + version;
+          (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(
+            '0x0000000000000000000000000000000000000000');
+        });
+      });
+    });
 
-  it('recover using web3.eth.sign() should return wrong signer', async function () {
-    // Create the signature
-    const signature = signMessage(anyone, TEST_MESSAGE);
+    context('with v1 signature', function () {
+      const signer = '0x1e318623ab09fe6de3c9b8672098464aeda9100e';
+      // eslint-disable-next-line max-len
+      const signatureWithoutVersion = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0';
 
-    // Recover the signer address from the generated message and wrong signature.
-    (await this.mock.recover(WRONG_MESSAGE, signature)).should.not.equal(anyone);
+      context('with 01 as version value', function () {
+        it('works', async function () {
+          const version = '01';
+          const signature = signatureWithoutVersion + version;
+          (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(signer);
+        });
+      });
+
+      context('with 28 signature', function () {
+        it('works', async function () {
+          const version = '1c'; // 28 = 1c.
+          const signature = signatureWithoutVersion + version;
+          (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(signer);
+        });
+      });
+
+      context('with wrong version', function () {
+        it('returns 0', async function () {
+          // The last two hex digits are the signature version.
+          // The only valid values are 0, 1, 27 and 28.
+          const version = '02';
+          const signature = signatureWithoutVersion + version;
+          (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(
+            '0x0000000000000000000000000000000000000000');
+        });
+      });
+    });
+
+    context('using web3.eth.sign', function () {
+      context('with correct signature', function () {
+        it('returns signer address', async function () {
+          // Create the signature
+          const signature = signMessage(anyone, TEST_MESSAGE);
+
+          // Recover the signer address from the generated message and signature.
+          (await this.ecdsa.recover(
+            toEthSignedMessageHash(TEST_MESSAGE),
+            signature
+          )).should.equal(anyone);
+        });
+      });
+
+      context('with wrong signature', function () {
+        it('does not return signer address', async function () {
+          // Create the signature
+          const signature = signMessage(anyone, TEST_MESSAGE);
+
+          // Recover the signer address from the generated message and wrong signature.
+          (await this.ecdsa.recover(WRONG_MESSAGE, signature)).should.not.equal(anyone);
+        });
+      });
+    });
   });
 
-  // @TODO - remove `skip` once we upgrade to solc^0.5
-  it.skip('recover should revert when a small hash is sent', async function () {
-    // Create the signature
-    const signature = signMessage(anyone, TEST_MESSAGE);
-    await expectThrow(
-      this.mock.recover(TEST_MESSAGE.substring(2), signature)
-    );
+  context('with small hash', function () {
+    // @TODO - remove `skip` once we upgrade to solc^0.5
+    it.skip('reverts', async function () {
+      // Create the signature
+      const signature = signMessage(anyone, TEST_MESSAGE);
+      await expectThrow(
+        this.ecdsa.recover(TEST_MESSAGE.substring(2), signature)
+      );
+    });
   });
 
   context('toEthSignedMessage', function () {
     it('should prefix hashes correctly', async function () {
-      (await this.mock.toEthSignedMessageHash(TEST_MESSAGE)).should.equal(toEthSignedMessageHash(TEST_MESSAGE));
+      (await this.ecdsa.toEthSignedMessageHash(TEST_MESSAGE)).should.equal(toEthSignedMessageHash(TEST_MESSAGE));
     });
   });
 });

test/payment/SplitPayment.test.js

@@ -51,10 +51,10 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1,
     });
 
     it('should have payees', async function () {
-      this.payees.forEach(async (payee, index) => {
-        (await this.payee(index)).should.be.equal(payee);
+      await Promise.all(this.payees.map(async (payee, index) => {
+        (await this.contract.payee(index)).should.be.equal(payee);
         (await this.contract.released(payee)).should.be.bignumber.equal(0);
-      });
+      }));
     });
 
     it('should accept payments', async function () {

test/token/ERC721/ERC721Full.test.js

 const { assertRevert } = require('../../helpers/assertRevert');
 const { shouldBehaveLikeERC721 } = require('./ERC721.behavior');
-const { shouldBehaveLikeMintAndBurnERC721 } = require('./ERC721MintBurn.behavior');
 const { shouldSupportInterfaces } = require('../../introspection/SupportsInterface.behavior');
 const _ = require('lodash');
 
@@ -217,7 +216,6 @@ contract('ERC721Full', function ([
   });
 
   shouldBehaveLikeERC721(creator, minter, accounts);
-  shouldBehaveLikeMintAndBurnERC721(creator, minter, accounts);
 
   shouldSupportInterfaces([
     'ERC165',

test/token/ERC721/ERC721MintBurn.behavior.js

@@ -52,13 +52,13 @@ function shouldBehaveLikeMintAndBurnERC721 (
 
       describe('when the given owner address is the zero address', function () {
         it('reverts', async function () {
-          await assertRevert(this.token.mint(ZERO_ADDRESS, thirdTokenId));
+          await assertRevert(this.token.mint(ZERO_ADDRESS, thirdTokenId, { from: minter }));
         });
       });
 
       describe('when the given token ID was already tracked by this contract', function () {
         it('reverts', async function () {
-          await assertRevert(this.token.mint(owner, firstTokenId));
+          await assertRevert(this.token.mint(owner, firstTokenId, { from: minter }));
         });
       });
     });