diff --git a/CHANGELOG.md b/CHANGELOG.md index e47d98e0b1f..03974ab302b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ * `ERC721`: added `_burn(uint256 tokenId)`, replacing the similar deprecated function (see below). ([#1550](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1550)) * `ERC721`: added `_tokensOfOwner(address owner)`, allowing to internally retrieve the array of an account's owned tokens. ([#1522](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1522)) * Crowdsales: all constructors are now `public`, meaning it is not necessary to extend these contracts in order to deploy them. The exception is `FinalizableCrowdsale`, since it is meaningless unless extended. ([#1564](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1564)) - * `SafeMath`: added overflow-safe operations for signed integers (`int256`). ([#1559](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1559)) + * `SignedSafeMath`: added overflow-safe operations for signed integers (`int256`). ([#1559](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1559), [#1588](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1588)) ### Improvements: * The compiler version required by `Array` was behind the rest of the libray so it was updated to `v0.4.24`. ([#1553](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1553)) diff --git a/contracts/drafts/SignedSafeMath.sol b/contracts/drafts/SignedSafeMath.sol new file mode 100644 index 00000000000..4e6ab0af7a3 --- /dev/null +++ b/contracts/drafts/SignedSafeMath.sol @@ -0,0 +1,60 @@ +pragma solidity ^0.5.0; + +/** + * @title SignedSafeMath + * @dev Signed math operations with safety checks that revert on error + */ +library SignedSafeMath { + int256 constant private INT256_MIN = -2**255; + + /** + * @dev Multiplies two signed integers, reverts on overflow. + */ + function mul(int256 a, int256 b) internal pure returns (int256) { + // Gas optimization: this is cheaper than requiring 'a' not being zero, but the + // benefit is lost if 'b' is also tested. + // See: https://github.com/OpenZeppelin/openzeppelin-solidity/pull/522 + if (a == 0) { + return 0; + } + + require(!(a == -1 && b == INT256_MIN)); // This is the only case of overflow not detected by the check below + + int256 c = a * b; + require(c / a == b); + + return c; + } + + /** + * @dev Integer division of two signed integers truncating the quotient, reverts on division by zero. + */ + function div(int256 a, int256 b) internal pure returns (int256) { + require(b != 0); // Solidity only automatically asserts when dividing by 0 + require(!(b == -1 && a == INT256_MIN)); // This is the only case of overflow + + int256 c = a / b; + + return c; + } + + /** + * @dev Subtracts two signed integers, reverts on overflow. + */ + function sub(int256 a, int256 b) internal pure returns (int256) { + int256 c = a - b; + require((b >= 0 && c <= a) || (b < 0 && c > a)); + + return c; + } + + /** + * @dev Adds two signed integers, reverts on overflow. + */ + function add(int256 a, int256 b) internal pure returns (int256) { + int256 c = a + b; + require((b >= 0 && c >= a) || (b < 0 && c < a)); + + return c; + } +} diff --git a/contracts/math/SafeMath.sol b/contracts/math/SafeMath.sol index 9d65897a0de..54ab43acf52 100644 --- a/contracts/math/SafeMath.sol +++ b/contracts/math/SafeMath.sol @@ -2,11 +2,9 @@ pragma solidity ^0.5.0; /** * @title SafeMath - * @dev Math operations with safety checks that revert on error + * @dev Unsigned math operations with safety checks that revert on error */ library SafeMath { - int256 constant private INT256_MIN = -2**255; - /** * @dev Multiplies two unsigned integers, reverts on overflow. */ @@ -24,25 +22,6 @@ library SafeMath { return c; } - /** - * @dev Multiplies two signed integers, reverts on overflow. - */ - function mul(int256 a, int256 b) internal pure returns (int256) { - // Gas optimization: this is cheaper than requiring 'a' not being zero, but the - // benefit is lost if 'b' is also tested. - // See: https://github.com/OpenZeppelin/openzeppelin-solidity/pull/522 - if (a == 0) { - return 0; - } - - require(!(a == -1 && b == INT256_MIN)); // This is the only case of overflow not detected by the check below - - int256 c = a * b; - require(c / a == b); - - return c; - } - /** * @dev Integer division of two unsigned integers truncating the quotient, reverts on division by zero. */ @@ -55,18 +34,6 @@ library SafeMath { return c; } - /** - * @dev Integer division of two signed integers truncating the quotient, reverts on division by zero. - */ - function div(int256 a, int256 b) internal pure returns (int256) { - require(b != 0); // Solidity only automatically asserts when dividing by 0 - require(!(b == -1 && a == INT256_MIN)); // This is the only case of overflow - - int256 c = a / b; - - return c; - } - /** * @dev Subtracts two unsigned integers, reverts on overflow (i.e. if subtrahend is greater than minuend). */ @@ -77,16 +44,6 @@ library SafeMath { return c; } - /** - * @dev Subtracts two signed integers, reverts on overflow. - */ - function sub(int256 a, int256 b) internal pure returns (int256) { - int256 c = a - b; - require((b >= 0 && c <= a) || (b < 0 && c > a)); - - return c; - } - /** * @dev Adds two unsigned integers, reverts on overflow. */ @@ -97,16 +54,6 @@ library SafeMath { return c; } - /** - * @dev Adds two signed integers, reverts on overflow. - */ - function add(int256 a, int256 b) internal pure returns (int256) { - int256 c = a + b; - require((b >= 0 && c >= a) || (b < 0 && c < a)); - - return c; - } - /** * @dev Divides two unsigned integers and returns the remainder (unsigned integer modulo), * reverts when dividing by zero. diff --git a/contracts/mocks/SafeMathMock.sol b/contracts/mocks/SafeMathMock.sol index b61683d3e06..43dac5ec29f 100644 --- a/contracts/mocks/SafeMathMock.sol +++ b/contracts/mocks/SafeMathMock.sol @@ -1,43 +1,25 @@ pragma solidity ^0.5.0; - import "../math/SafeMath.sol"; - contract SafeMathMock { - function mulUints(uint256 a, uint256 b) public pure returns (uint256) { + function mul(uint256 a, uint256 b) public pure returns (uint256) { return SafeMath.mul(a, b); } - function mulInts(int256 a, int256 b) public pure returns (int256) { - return SafeMath.mul(a, b); - } - - function divUints(uint256 a, uint256 b) public pure returns (uint256) { - return SafeMath.div(a, b); - } - - function divInts(int256 a, int256 b) public pure returns (int256) { + function div(uint256 a, uint256 b) public pure returns (uint256) { return SafeMath.div(a, b); } - function subUints(uint256 a, uint256 b) public pure returns (uint256) { - return SafeMath.sub(a, b); - } - - function subInts(int256 a, int256 b) public pure returns (int256) { + function sub(uint256 a, uint256 b) public pure returns (uint256) { return SafeMath.sub(a, b); } - function addUints(uint256 a, uint256 b) public pure returns (uint256) { - return SafeMath.add(a, b); - } - - function addInts(int256 a, int256 b) public pure returns (int256) { + function add(uint256 a, uint256 b) public pure returns (uint256) { return SafeMath.add(a, b); } - function modUints(uint256 a, uint256 b) public pure returns (uint256) { + function mod(uint256 a, uint256 b) public pure returns (uint256) { return SafeMath.mod(a, b); } } diff --git a/contracts/mocks/SignedSafeMathMock.sol b/contracts/mocks/SignedSafeMathMock.sol new file mode 100644 index 00000000000..90a3ee642bc --- /dev/null +++ b/contracts/mocks/SignedSafeMathMock.sol @@ -0,0 +1,21 @@ +pragma solidity ^0.5.0; + +import "../drafts/SignedSafeMath.sol"; + +contract SignedSafeMathMock { + function mul(int256 a, int256 b) public pure returns (int256) { + return SignedSafeMath.mul(a, b); + } + + function div(int256 a, int256 b) public pure returns (int256) { + return SignedSafeMath.div(a, b); + } + + function sub(int256 a, int256 b) public pure returns (int256) { + return SignedSafeMath.sub(a, b); + } + + function add(int256 a, int256 b) public pure returns (int256) { + return SignedSafeMath.add(a, b); + } +} diff --git a/test/drafts/SignedSafeMath.test.js b/test/drafts/SignedSafeMath.test.js new file mode 100644 index 00000000000..e144d39f6d5 --- /dev/null +++ b/test/drafts/SignedSafeMath.test.js @@ -0,0 +1,138 @@ +const shouldFail = require('../helpers/shouldFail'); +const { MIN_INT256, MAX_INT256 } = require('../helpers/constants'); + +const SignedSafeMathMock = artifacts.require('SignedSafeMathMock'); + +const { BigNumber } = require('../helpers/setup'); + +contract('SignedSafeMath', function () { + beforeEach(async function () { + this.safeMath = await SignedSafeMathMock.new(); + }); + + describe('add', function () { + it('adds correctly if it does not overflow and the result is positve', async function () { + const a = new BigNumber(1234); + const b = new BigNumber(5678); + + (await this.safeMath.add(a, b)).should.be.bignumber.equal(a.plus(b)); + }); + + it('adds correctly if it does not overflow and the result is negative', async function () { + const a = MAX_INT256; + const b = MIN_INT256; + + const result = await this.safeMath.add(a, b); + result.should.be.bignumber.equal(a.plus(b)); + }); + + it('reverts on positive addition overflow', async function () { + const a = MAX_INT256; + const b = new BigNumber(1); + + await shouldFail.reverting(this.safeMath.add(a, b)); + }); + + it('reverts on negative addition overflow', async function () { + const a = MIN_INT256; + const b = new BigNumber(-1); + + await shouldFail.reverting(this.safeMath.add(a, b)); + }); + }); + + describe('sub', function () { + it('subtracts correctly if it does not overflow and the result is positive', async function () { + const a = new BigNumber(5678); + const b = new BigNumber(1234); + + const result = await this.safeMath.sub(a, b); + result.should.be.bignumber.equal(a.minus(b)); + }); + + it('subtracts correctly if it does not overflow and the result is negative', async function () { + const a = new BigNumber(1234); + const b = new BigNumber(5678); + + const result = await this.safeMath.sub(a, b); + result.should.be.bignumber.equal(a.minus(b)); + }); + + it('reverts on positive subtraction overflow', async function () { + const a = MAX_INT256; + const b = new BigNumber(-1); + + await shouldFail.reverting(this.safeMath.sub(a, b)); + }); + + it('reverts on negative subtraction overflow', async function () { + const a = MIN_INT256; + const b = new BigNumber(1); + + await shouldFail.reverting(this.safeMath.sub(a, b)); + }); + }); + + describe('mul', function () { + it('multiplies correctly', async function () { + const a = new BigNumber(5678); + const b = new BigNumber(-1234); + + const result = await this.safeMath.mul(a, b); + result.should.be.bignumber.equal(a.times(b)); + }); + + it('handles a zero product correctly', async function () { + const a = new BigNumber(0); + const b = new BigNumber(5678); + + const result = await this.safeMath.mul(a, b); + result.should.be.bignumber.equal(a.times(b)); + }); + + it('reverts on multiplication overflow, positive operands', async function () { + const a = MAX_INT256; + const b = new BigNumber(2); + + await shouldFail.reverting(this.safeMath.mul(a, b)); + }); + + it('reverts when minimum integer is multiplied by -1', async function () { + const a = MIN_INT256; + const b = new BigNumber(-1); + + await shouldFail.reverting(this.safeMath.mul(a, b)); + }); + + it('reverts when -1 is multiplied by minimum integer', async function () { + const a = new BigNumber(-1); + const b = MIN_INT256; + + await shouldFail.reverting(this.safeMath.mul(a, b)); + }); + }); + + describe('div', function () { + it('divides correctly', async function () { + const a = new BigNumber(-5678); + const b = new BigNumber(5678); + + const result = await this.safeMath.div(a, b); + result.should.be.bignumber.equal(a.div(b)); + }); + + it('reverts on zero division', async function () { + const a = new BigNumber(-5678); + const b = new BigNumber(0); + + await shouldFail.reverting(this.safeMath.div(a, b)); + }); + + it('reverts on overflow, negative second', async function () { + const a = new BigNumber(MIN_INT256); + const b = new BigNumber(-1); + + await shouldFail.reverting(this.safeMath.div(a, b)); + }); + }); +}); diff --git a/test/math/SafeMath.test.js b/test/math/SafeMath.test.js index e2cf371efa5..221c998ffd8 100644 --- a/test/math/SafeMath.test.js +++ b/test/math/SafeMath.test.js @@ -1,5 +1,5 @@ const shouldFail = require('../helpers/shouldFail'); -const { MAX_UINT256, MIN_INT256, MAX_INT256 } = require('../helpers/constants'); +const { MAX_UINT256 } = require('../helpers/constants'); const SafeMathMock = artifacts.require('SafeMathMock'); @@ -10,264 +10,134 @@ contract('SafeMath', function () { this.safeMath = await SafeMathMock.new(); }); - describe('unsigned', function () { - describe('add', function () { - it('adds correctly', async function () { - const a = new BigNumber(5678); - const b = new BigNumber(1234); - - (await this.safeMath.addUints(a, b)).should.be.bignumber.equal(a.plus(b)); - }); + describe('add', function () { + it('adds correctly', async function () { + const a = new BigNumber(5678); + const b = new BigNumber(1234); - it('reverts on addition overflow', async function () { - const a = MAX_UINT256; - const b = new BigNumber(1); - - await shouldFail.reverting(this.safeMath.addUints(a, b)); - }); + (await this.safeMath.add(a, b)).should.be.bignumber.equal(a.plus(b)); }); - describe('sub', function () { - it('subtracts correctly', async function () { - const a = new BigNumber(5678); - const b = new BigNumber(1234); + it('reverts on addition overflow', async function () { + const a = MAX_UINT256; + const b = new BigNumber(1); - (await this.safeMath.subUints(a, b)).should.be.bignumber.equal(a.minus(b)); - }); - - it('reverts if subtraction result would be negative', async function () { - const a = new BigNumber(1234); - const b = new BigNumber(5678); - - await shouldFail.reverting(this.safeMath.subUints(a, b)); - }); + await shouldFail.reverting(this.safeMath.add(a, b)); }); + }); - describe('mul', function () { - it('multiplies correctly', async function () { - const a = new BigNumber(1234); - const b = new BigNumber(5678); - - (await this.safeMath.mulUints(a, b)).should.be.bignumber.equal(a.times(b)); - }); - - it('handles a zero product correctly (first number as zero)', async function () { - const a = new BigNumber(0); - const b = new BigNumber(5678); - - (await this.safeMath.mulUints(a, b)).should.be.bignumber.equal(a.times(b)); - }); - - it('handles a zero product correctly (second number as zero)', async function () { - const a = new BigNumber(5678); - const b = new BigNumber(0); - - (await this.safeMath.mulUints(a, b)).should.be.bignumber.equal(a.times(b)); - }); - - it('reverts on multiplication overflow', async function () { - const a = MAX_UINT256; - const b = new BigNumber(2); + describe('sub', function () { + it('subtracts correctly', async function () { + const a = new BigNumber(5678); + const b = new BigNumber(1234); - await shouldFail.reverting(this.safeMath.mulUints(a, b)); - }); + (await this.safeMath.sub(a, b)).should.be.bignumber.equal(a.minus(b)); }); - describe('div', function () { - it('divides correctly', async function () { - const a = new BigNumber(5678); - const b = new BigNumber(5678); - - (await this.safeMath.divUints(a, b)).should.be.bignumber.equal(a.div(b)); - }); - - it('divides zero correctly', async function () { - const a = new BigNumber(0); - const b = new BigNumber(5678); - - (await this.safeMath.divUints(a, b)).should.be.bignumber.equal(0); - }); - - it('returns complete number result on non-even division', async function () { - const a = new BigNumber(7000); - const b = new BigNumber(5678); - - (await this.safeMath.divUints(a, b)).should.be.bignumber.equal(1); - }); + it('reverts if subtraction result would be negative', async function () { + const a = new BigNumber(1234); + const b = new BigNumber(5678); - it('reverts on zero division', async function () { - const a = new BigNumber(5678); - const b = new BigNumber(0); - - await shouldFail.reverting(this.safeMath.divUints(a, b)); - }); + await shouldFail.reverting(this.safeMath.sub(a, b)); }); + }); - describe('mod', function () { - describe('modulos correctly', async function () { - it('when the dividend is smaller than the divisor', async function () { - const a = new BigNumber(284); - const b = new BigNumber(5678); - - (await this.safeMath.modUints(a, b)).should.be.bignumber.equal(a.mod(b)); - }); - - it('when the dividend is equal to the divisor', async function () { - const a = new BigNumber(5678); - const b = new BigNumber(5678); + describe('mul', function () { + it('multiplies correctly', async function () { + const a = new BigNumber(1234); + const b = new BigNumber(5678); - (await this.safeMath.modUints(a, b)).should.be.bignumber.equal(a.mod(b)); - }); + (await this.safeMath.mul(a, b)).should.be.bignumber.equal(a.times(b)); + }); - it('when the dividend is larger than the divisor', async function () { - const a = new BigNumber(7000); - const b = new BigNumber(5678); + it('handles a zero product correctly (first number as zero)', async function () { + const a = new BigNumber(0); + const b = new BigNumber(5678); - (await this.safeMath.modUints(a, b)).should.be.bignumber.equal(a.mod(b)); - }); + (await this.safeMath.mul(a, b)).should.be.bignumber.equal(a.times(b)); + }); - it('when the dividend is a multiple of the divisor', async function () { - const a = new BigNumber(17034); // 17034 == 5678 * 3 - const b = new BigNumber(5678); + it('handles a zero product correctly (second number as zero)', async function () { + const a = new BigNumber(5678); + const b = new BigNumber(0); - (await this.safeMath.modUints(a, b)).should.be.bignumber.equal(a.mod(b)); - }); - }); + (await this.safeMath.mul(a, b)).should.be.bignumber.equal(a.times(b)); + }); - it('reverts with a 0 divisor', async function () { - const a = new BigNumber(5678); - const b = new BigNumber(0); + it('reverts on multiplication overflow', async function () { + const a = MAX_UINT256; + const b = new BigNumber(2); - await shouldFail.reverting(this.safeMath.modUints(a, b)); - }); + await shouldFail.reverting(this.safeMath.mul(a, b)); }); }); - describe('signed', function () { - describe('add', function () { - it('adds correctly if it does not overflow and the result is positve', async function () { - const a = new BigNumber(1234); - const b = new BigNumber(5678); - - (await this.safeMath.addUints(a, b)).should.be.bignumber.equal(a.plus(b)); - }); + describe('div', function () { + it('divides correctly', async function () { + const a = new BigNumber(5678); + const b = new BigNumber(5678); - it('adds correctly if it does not overflow and the result is negative', async function () { - const a = MAX_INT256; - const b = MIN_INT256; - - const result = await this.safeMath.addInts(a, b); - result.should.be.bignumber.equal(a.plus(b)); - }); + (await this.safeMath.div(a, b)).should.be.bignumber.equal(a.div(b)); + }); - it('reverts on positive addition overflow', async function () { - const a = MAX_INT256; - const b = new BigNumber(1); + it('divides zero correctly', async function () { + const a = new BigNumber(0); + const b = new BigNumber(5678); - await shouldFail.reverting(this.safeMath.addInts(a, b)); - }); + (await this.safeMath.div(a, b)).should.be.bignumber.equal(0); + }); - it('reverts on negative addition overflow', async function () { - const a = MIN_INT256; - const b = new BigNumber(-1); + it('returns complete number result on non-even division', async function () { + const a = new BigNumber(7000); + const b = new BigNumber(5678); - await shouldFail.reverting(this.safeMath.addInts(a, b)); - }); + (await this.safeMath.div(a, b)).should.be.bignumber.equal(1); }); - describe('sub', function () { - it('subtracts correctly if it does not overflow and the result is positive', async function () { - const a = new BigNumber(5678); - const b = new BigNumber(1234); + it('reverts on zero division', async function () { + const a = new BigNumber(5678); + const b = new BigNumber(0); - const result = await this.safeMath.subInts(a, b); - result.should.be.bignumber.equal(a.minus(b)); - }); + await shouldFail.reverting(this.safeMath.div(a, b)); + }); + }); - it('subtracts correctly if it does not overflow and the result is negative', async function () { - const a = new BigNumber(1234); + describe('mod', function () { + describe('modulos correctly', async function () { + it('when the dividend is smaller than the divisor', async function () { + const a = new BigNumber(284); const b = new BigNumber(5678); - const result = await this.safeMath.subInts(a, b); - result.should.be.bignumber.equal(a.minus(b)); - }); - - it('reverts on positive subtraction overflow', async function () { - const a = MAX_INT256; - const b = new BigNumber(-1); - - await shouldFail.reverting(this.safeMath.subInts(a, b)); + (await this.safeMath.mod(a, b)).should.be.bignumber.equal(a.mod(b)); }); - it('reverts on negative subtraction overflow', async function () { - const a = MIN_INT256; - const b = new BigNumber(1); - - await shouldFail.reverting(this.safeMath.subInts(a, b)); - }); - }); - - describe('mul', function () { - it('multiplies correctly', async function () { + it('when the dividend is equal to the divisor', async function () { const a = new BigNumber(5678); - const b = new BigNumber(-1234); - - const result = await this.safeMath.mulInts(a, b); - result.should.be.bignumber.equal(a.times(b)); - }); - - it('handles a zero product correctly', async function () { - const a = new BigNumber(0); const b = new BigNumber(5678); - const result = await this.safeMath.mulInts(a, b); - result.should.be.bignumber.equal(a.times(b)); + (await this.safeMath.mod(a, b)).should.be.bignumber.equal(a.mod(b)); }); - it('reverts on multiplication overflow, positive operands', async function () { - const a = MAX_INT256; - const b = new BigNumber(2); - - await shouldFail.reverting(this.safeMath.mulInts(a, b)); - }); - - it('reverts when minimum integer is multiplied by -1', async function () { - const a = MIN_INT256; - const b = new BigNumber(-1); - - await shouldFail.reverting(this.safeMath.mulInts(a, b)); - }); - - it('reverts when -1 is multiplied by minimum integer', async function () { - const a = new BigNumber(-1); - const b = MIN_INT256; - - await shouldFail.reverting(this.safeMath.mulInts(a, b)); - }); - }); - - describe('div', function () { - it('divides correctly', async function () { - const a = new BigNumber(-5678); + it('when the dividend is larger than the divisor', async function () { + const a = new BigNumber(7000); const b = new BigNumber(5678); - const result = await this.safeMath.divInts(a, b); - result.should.be.bignumber.equal(a.div(b)); + (await this.safeMath.mod(a, b)).should.be.bignumber.equal(a.mod(b)); }); - it('reverts on zero division', async function () { - const a = new BigNumber(-5678); - const b = new BigNumber(0); + it('when the dividend is a multiple of the divisor', async function () { + const a = new BigNumber(17034); // 17034 == 5678 * 3 + const b = new BigNumber(5678); - await shouldFail.reverting(this.safeMath.divInts(a, b)); + (await this.safeMath.mod(a, b)).should.be.bignumber.equal(a.mod(b)); }); + }); - it('reverts on overflow, negative second', async function () { - const a = new BigNumber(MIN_INT256); - const b = new BigNumber(-1); + it('reverts with a 0 divisor', async function () { + const a = new BigNumber(5678); + const b = new BigNumber(0); - await shouldFail.reverting(this.safeMath.divInts(a, b)); - }); + await shouldFail.reverting(this.safeMath.mod(a, b)); }); }); });