diff --git a/contracts/staking/contracts/src/libs/LibFixedMath.sol b/contracts/staking/contracts/src/libs/LibFixedMath.sol index bab8d2db75..dc5ed65802 100644 --- a/contracts/staking/contracts/src/libs/LibFixedMath.sol +++ b/contracts/staking/contracts/src/libs/LibFixedMath.sol @@ -27,6 +27,8 @@ library LibFixedMath { // 1 int256 private constant FIXED_1 = int256(0x0000000000000000000000000000000080000000000000000000000000000000); + // 2**255 + int256 private constant MIN_FIXED_VAL = int256(0x8000000000000000000000000000000000000000000000000000000000000000); // 1^2 (in fixed-point) int256 private constant FIXED_1_SQUARED = int256(0x4000000000000000000000000000000000000000000000000000000000000000); // 1 @@ -50,6 +52,12 @@ library LibFixedMath { /// @dev Returns the addition of two fixed point numbers, reverting on overflow. function sub(int256 a, int256 b) internal pure returns (int256 c) { + if (b == MIN_FIXED_VAL) { + LibRichErrors.rrevert(LibFixedMathRichErrors.SignedValueError( + LibFixedMathRichErrors.ValueErrorCodes.TOO_SMALL, + b + )); + } c = _add(a, -b); } @@ -359,14 +367,7 @@ library LibFixedMath { /// @dev Adds two numbers, reverting on overflow. function _add(int256 a, int256 b) private pure returns (int256 c) { c = a + b; - if (c > 0 && a < 0 && b < 0) { - LibRichErrors.rrevert(LibFixedMathRichErrors.BinOpError( - LibFixedMathRichErrors.BinOpErrorCodes.SUBTRACTION_OVERFLOW, - a, - b - )); - } - if (c < 0 && a > 0 && b > 0) { + if ((a < 0 && b < 0 && c > a) || (a > 0 && b > 0 && c < a)) { LibRichErrors.rrevert(LibFixedMathRichErrors.BinOpError( LibFixedMathRichErrors.BinOpErrorCodes.ADDITION_OVERFLOW, a, diff --git a/contracts/staking/contracts/src/libs/LibFixedMathRichErrors.sol b/contracts/staking/contracts/src/libs/LibFixedMathRichErrors.sol index 8ea570bb87..4a4f111fcb 100644 --- a/contracts/staking/contracts/src/libs/LibFixedMathRichErrors.sol +++ b/contracts/staking/contracts/src/libs/LibFixedMathRichErrors.sol @@ -30,7 +30,6 @@ library LibFixedMathRichErrors { enum BinOpErrorCodes { ADDITION_OVERFLOW, - SUBTRACTION_OVERFLOW, MULTIPLICATION_OVERFLOW, DIVISION_BY_ZERO } diff --git a/contracts/staking/test/unit_tests/lib_fixed_math_test.ts b/contracts/staking/test/unit_tests/lib_fixed_math_test.ts index 5e1eae7aff..eebfeaecbe 100644 --- a/contracts/staking/test/unit_tests/lib_fixed_math_test.ts +++ b/contracts/staking/test/unit_tests/lib_fixed_math_test.ts @@ -7,7 +7,7 @@ import { artifacts, TestLibFixedMathContract } from '../../src'; import { assertRoughlyEquals, fromFixed, toDecimal, toFixed } from '../utils/number_utils'; -blockchainTests('LibFixedMath unit tests', env => { +blockchainTests.only('LibFixedMath unit tests', env => { let testContract: TestLibFixedMathContract; before(async () => { @@ -131,6 +131,19 @@ blockchainTests('LibFixedMath unit tests', env => { const tx = testContract.mulDiv.callAsync(toFixed(a), new BigNumber(n), new BigNumber(d)); return expect(tx).to.revertWith(expectedError); }); + + it('int(-1) * int(1) / int(-1) == int(1)', async () => { + const [a, n, d] = [-1, 1, -1]; + const r = await testContract.mulDiv.callAsync(new BigNumber(a), new BigNumber(n), new BigNumber(d)); + assertFixedEquals(r, fromFixed(1)); + }); + + it('-1 * int(1) / int(-1) == 1', async () => { + const [a, n, d] = [-1, 1, -1]; + const r = await testContract.mulDiv.callAsync(toFixed(a), new BigNumber(n), new BigNumber(d)); + assertFixedEquals(r, 1); + }); + }); describe('add()', () => { @@ -170,13 +183,41 @@ blockchainTests('LibFixedMath unit tests', env => { it('throws on underflow', async () => { const [a, b] = [MIN_FIXED_VALUE, new BigNumber(-1)]; const expectedError = new FixedMathRevertErrors.BinOpError( - FixedMathRevertErrors.BinOpErrorCodes.SubtractionUnderflow, + FixedMathRevertErrors.BinOpErrorCodes.AdditionOverflow, a, b, ); const tx = testContract.add.callAsync(a, b); return expect(tx).to.revertWith(expectedError); }); + + it('MIN_FIXED + MIN_FIXED throws', async () => { + const [a, b] = [MIN_FIXED_VALUE, MIN_FIXED_VALUE]; + const expectedError = new FixedMathRevertErrors.BinOpError( + FixedMathRevertErrors.BinOpErrorCodes.AdditionOverflow, + a, + b, + ); + const tx = testContract.add.callAsync(a, b); + return expect(tx).to.revertWith(expectedError); + }); + + it('MAX_FIXED + MAX_FIXED throws', async () => { + const [a, b] = [MAX_FIXED_VALUE, MAX_FIXED_VALUE]; + const expectedError = new FixedMathRevertErrors.BinOpError( + FixedMathRevertErrors.BinOpErrorCodes.AdditionOverflow, + a, + b, + ); + const tx = testContract.add.callAsync(a, b); + return expect(tx).to.revertWith(expectedError); + }); + + it('MIN_FIXED + MAX_FIXED == int(-1)', async () => { + const [a, b] = [MIN_FIXED_VALUE, MAX_FIXED_VALUE]; + const r = await testContract.add.callAsync(a, b); + expect(r).to.bignumber.eq(-1); + }); }); describe('sub()', () => { @@ -205,7 +246,7 @@ blockchainTests('LibFixedMath unit tests', env => { it('throws on underflow', async () => { const [a, b] = [MIN_FIXED_VALUE, new BigNumber(1)]; const expectedError = new FixedMathRevertErrors.BinOpError( - FixedMathRevertErrors.BinOpErrorCodes.SubtractionUnderflow, + FixedMathRevertErrors.BinOpErrorCodes.AdditionOverflow, a, b.negated(), ); @@ -223,6 +264,46 @@ blockchainTests('LibFixedMath unit tests', env => { const tx = testContract.sub.callAsync(a, b); return expect(tx).to.revertWith(expectedError); }); + + it('MIN_FIXED - MIN_FIXED throws', async () => { + const [a, b] = [MIN_FIXED_VALUE, MIN_FIXED_VALUE]; + // This fails because `-MIN_FIXED_VALUE == MIN_FIXED_VALUE` because of + // twos-complement. + const expectedError = new FixedMathRevertErrors.BinOpError( + FixedMathRevertErrors.BinOpErrorCodes.AdditionOverflow, + a, + b, + ); + const tx = testContract.sub.callAsync(a, b); + return expect(tx).to.revertWith(expectedError); + }); + + it('MAX_FIXED - MAX_FIXED == 0', async () => { + const [a, b] = [MAX_FIXED_VALUE, MAX_FIXED_VALUE]; + const r = await testContract.sub.callAsync(a, b); + return expect(r).to.bignumber.eq(0); + }); + + it('MIN_FIXED - MAX_FIXED throws', async () => { + const [a, b] = [MIN_FIXED_VALUE, MAX_FIXED_VALUE]; + const expectedError = new FixedMathRevertErrors.BinOpError( + FixedMathRevertErrors.BinOpErrorCodes.AdditionOverflow, + a, + b.negated(), + ); + const tx = testContract.sub.callAsync(a, b); + return expect(tx).to.revertWith(expectedError); + }); + + it('MAX_FIXED - MIN_FIXED throws', async () => { + const [a, b] = [MAX_FIXED_VALUE, MIN_FIXED_VALUE]; + const expectedError = new FixedMathRevertErrors.SignedValueError( + FixedMathRevertErrors.ValueErrorCodes.TooSmall, + b, + ); + const tx = testContract.sub.callAsync(a, b); + return expect(tx).to.revertWith(expectedError); + }); }); describe('mul()', () => { diff --git a/packages/utils/src/fixed_math_revert_errors.ts b/packages/utils/src/fixed_math_revert_errors.ts index b405653e6d..0782600611 100644 --- a/packages/utils/src/fixed_math_revert_errors.ts +++ b/packages/utils/src/fixed_math_revert_errors.ts @@ -10,7 +10,6 @@ export enum ValueErrorCodes { export enum BinOpErrorCodes { AdditionOverflow, - SubtractionUnderflow, MultiplicationOverflow, DivisionByZero, }