@0x/contracts-staking: Implement better overflow detection in LibFixedMath.

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) {
+        if ((a < 0 && b < 0 && c > a) || (a > 0 && b > 0 && c < a)) {
-            LibRichErrors.rrevert(LibFixedMathRichErrors.BinOpError(
-                LibFixedMathRichErrors.BinOpErrorCodes.SUBTRACTION_OVERFLOW,
-                a,
-                b
-            ));
-        }
-        if (c < 0 && a > 0 && b > 0) {
             LibRichErrors.rrevert(LibFixedMathRichErrors.BinOpError(
                 LibFixedMathRichErrors.BinOpErrorCodes.ADDITION_OVERFLOW,
                 a,

contracts/staking/contracts/src/libs/LibFixedMathRichErrors.sol

@@ -30,7 +30,6 @@ library LibFixedMathRichErrors {
 
     enum BinOpErrorCodes {
         ADDITION_OVERFLOW,
-        SUBTRACTION_OVERFLOW,
         MULTIPLICATION_OVERFLOW,
         DIVISION_BY_ZERO
     }

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()', () => {

packages/utils/src/fixed_math_revert_errors.ts

@@ -10,7 +10,6 @@ export enum ValueErrorCodes {
 
 export enum BinOpErrorCodes {
     AdditionOverflow,
-    SubtractionUnderflow,
     MultiplicationOverflow,
     DivisionByZero,
 }