diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index c32ba08f2..72b331668 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.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`). diff --git a/contracts/drafts/BreakInvariantBounty.sol b/contracts/drafts/BreakInvariantBounty.sol index 8b039dedc..bbe25d9fa 100644 --- a/contracts/drafts/BreakInvariantBounty.sol +++ b/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 diff --git a/contracts/mocks/ERC721FullMock.sol b/contracts/mocks/ERC721FullMock.sol index d6ae649e5..099a3325a 100644 --- a/contracts/mocks/ERC721FullMock.sol +++ b/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); diff --git a/contracts/mocks/ERC721MintableBurnableImpl.sol b/contracts/mocks/ERC721MintableBurnableImpl.sol index b72736f74..4a3896bbd 100644 --- a/contracts/mocks/ERC721MintableBurnableImpl.sol +++ b/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 diff --git a/contracts/mocks/SignatureBouncerMock.sol b/contracts/mocks/SignatureBouncerMock.sol index f1815203e..fb143a041 100644 --- a/contracts/mocks/SignatureBouncerMock.sol +++ b/contracts/mocks/SignatureBouncerMock.sol @@ -69,4 +69,11 @@ contract SignatureBouncerMock is Initializable, SignatureBouncer, SignerRoleMock { } + + function tooShortMsgData() + public + onlyValidSignatureAndData("") + view + { + } } diff --git a/contracts/payment/RefundEscrow.sol b/contracts/payment/RefundEscrow.sol index 5bcc66526..53a003fd0 100644 --- a/contracts/payment/RefundEscrow.sol +++ b/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(); diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index 673396e27..86a67df69 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/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; } @@ -159,6 +150,21 @@ contract ERC20 is Initializable, IERC20 { return true; } + /** + * @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 diff --git a/contracts/token/ERC20/ERC20Burnable.sol b/contracts/token/ERC20/ERC20Burnable.sol index 78dcdfb4e..ed8fda3b1 100644 --- a/contracts/token/ERC20/ERC20Burnable.sol +++ b/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); - } } diff --git a/contracts/token/ERC721/ERC721MetadataMintable.sol b/contracts/token/ERC721/ERC721MetadataMintable.sol new file mode 100644 index 000000000..95a9f2124 --- /dev/null +++ b/contracts/token/ERC721/ERC721MetadataMintable.sol @@ -0,0 +1,32 @@ +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; + } +} diff --git a/contracts/token/ERC721/ERC721Mintable.sol b/contracts/token/ERC721/ERC721Mintable.sol index 331a24f89..8a5c559df 100644 --- a/contracts/token/ERC721/ERC721Mintable.sol +++ b/contracts/token/ERC721/ERC721Mintable.sol @@ -1,7 +1,7 @@ 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; - } } diff --git a/ethpm.json b/ethpm.json index f47982ffa..ced48af3d 100644 --- a/ethpm.json +++ b/ethpm.json @@ -1,6 +1,6 @@ { "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 " diff --git a/package-lock.json b/package-lock.json index 245fc1aa0..227e48835 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "openzeppelin-zos", - "version": "2.0.0-rc.1", + "version": "2.0.0-rc.3", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 5bf029abf..7ae04610c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "openzeppelin-zos", - "version": "2.0.0-rc.1", + "version": "2.0.0-rc.3", "description": "Secure Smart Contract library for Solidity", "files": [ "build", diff --git a/test/drafts/BreakInvariantBounty.test.js b/test/drafts/BreakInvariantBounty.test.js index d80bca92a..037ff53ef 100644 --- a/test/drafts/BreakInvariantBounty.test.js +++ b/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 })); + }); }); }); }); diff --git a/test/drafts/SignatureBouncer.test.js b/test/drafts/SignatureBouncer.test.js index b320ca98c..c234687fc 100644 --- a/test/drafts/SignatureBouncer.test.js +++ b/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 }) + ); + }); }); }); diff --git a/test/library/ECDSA.test.js b/test/library/ECDSA.test.js index 7fe705653..f8d333a5f 100644 --- a/test/library/ECDSA.test.js +++ b/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'; + + 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); + }); + }); + + 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); + }); + }); + + 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('with v1 signature', function () { + const signer = '0x1e318623ab09fe6de3c9b8672098464aeda9100e'; + // eslint-disable-next-line max-len + const signatureWithoutVersion = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0'; + + 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); + }); + }); + }); }); - 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); - }); - - it('recover using web3.eth.sign()', async function () { - // Create the signature - const signature = signMessage(anyone, TEST_MESSAGE); - - // Recover the signer address from the generated message and signature. - (await this.mock.recover( - toEthSignedMessageHash(TEST_MESSAGE), - signature - )).should.equal(anyone); - }); - - it('recover using web3.eth.sign() should return wrong signer', async function () { - // Create the signature - const signature = signMessage(anyone, TEST_MESSAGE); - - // Recover the signer address from the generated message and wrong signature. - (await this.mock.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)); }); }); }); diff --git a/test/payment/SplitPayment.test.js b/test/payment/SplitPayment.test.js index e5a5f1354..bb5aad0e9 100644 --- a/test/payment/SplitPayment.test.js +++ b/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 () { diff --git a/test/token/ERC721/ERC721Full.test.js b/test/token/ERC721/ERC721Full.test.js index ce8e915fa..ef3f67bc8 100644 --- a/test/token/ERC721/ERC721Full.test.js +++ b/test/token/ERC721/ERC721Full.test.js @@ -1,6 +1,5 @@ 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', diff --git a/test/token/ERC721/ERC721MintBurn.behavior.js b/test/token/ERC721/ERC721MintBurn.behavior.js index 31db1d176..ab54a6ae6 100644 --- a/test/token/ERC721/ERC721MintBurn.behavior.js +++ b/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 })); }); }); });