Merge pull request #2255 from 0xProject/fix/3.0-audit/staking/LibFixedMath-arithmetic-overflows

Fix LibFixedMath arithmetic overflows

contracts/staking/CHANGELOG.json

@@ -1,4 +1,13 @@
 [
+    {
+        "version": "1.1.0-beta.1",
+        "changes": [
+            {
+                "note": "Add more overflow safeguards to `LibFixedMath`",
+                "pr": 2255
+            }
+        ]
+    },
     {
         "version": "1.1.0-beta.0",
         "changes": [

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);
     }
 
@@ -87,6 +95,12 @@ library LibFixedMath {
 
     /// @dev Returns the absolute value of a fixed point number.
     function abs(int256 f) internal pure returns (int256 c) {
+        if (f == MIN_FIXED_VAL) {
+            LibRichErrors.rrevert(LibFixedMathRichErrors.SignedValueError(
+                LibFixedMathRichErrors.ValueErrorCodes.TOO_SMALL,
+                f
+            ));
+        }
         if (f >= 0) {
             c = f;
         } else {
@@ -353,20 +367,20 @@ library LibFixedMath {
                 b
             ));
         }
+        if (a == MIN_FIXED_VAL && b == -1) {
+            LibRichErrors.rrevert(LibFixedMathRichErrors.BinOpError(
+                LibFixedMathRichErrors.BinOpErrorCodes.DIVISION_OVERFLOW,
+                a,
+                b
+            ));
+        }
         c = a / b;
     }
 
     /// @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,9 +30,9 @@ library LibFixedMathRichErrors {
 
     enum BinOpErrorCodes {
         ADDITION_OVERFLOW,
-        SUBTRACTION_OVERFLOW,
         MULTIPLICATION_OVERFLOW,
-        DIVISION_BY_ZERO
+        DIVISION_BY_ZERO,
+        DIVISION_OVERFLOW
     }
 
     // bytes4(keccak256("SignedValueError(uint8,int256)"))

contracts/staking/test/unit_tests/lib_fixed_math_test.ts

@@ -21,6 +21,7 @@ blockchainTests('LibFixedMath unit tests', env => {
 
     const BITS_OF_PRECISION = 127;
     const FIXED_POINT_DIVISOR = new BigNumber(2).pow(BITS_OF_PRECISION);
+    const FIXED_1 = FIXED_POINT_DIVISOR;
     const MAX_FIXED_VALUE = new BigNumber(2).pow(255).minus(1);
     const MIN_FIXED_VALUE = new BigNumber(2).pow(255).times(-1);
     const MIN_EXP_NUMBER = new BigNumber('-63.875');
@@ -60,7 +61,35 @@ blockchainTests('LibFixedMath unit tests', env => {
         it('abs(0) == 0', async () => {
             const n = 0;
             const r = await testContract.abs.callAsync(toFixed(n));
-            assertFixedEquals(r, n);
+            expect(r).to.bignumber.eq(0);
+        });
+
+        it('abs(MAX_FIXED) == MAX_FIXED', async () => {
+            const n = MAX_FIXED_VALUE;
+            const r = await testContract.abs.callAsync(n);
+            expect(r).to.bignumber.eq(n);
+        });
+
+        it('abs(MIN_FIXED) throws', async () => {
+            const n = MIN_FIXED_VALUE;
+            const expectedError = new FixedMathRevertErrors.SignedValueError(
+                FixedMathRevertErrors.ValueErrorCodes.TooSmall,
+                n,
+            );
+            const tx = testContract.abs.callAsync(n);
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        it('abs(int(-1)) == int(1)', async () => {
+            const n = -1;
+            const r = await testContract.abs.callAsync(new BigNumber(n));
+            expect(r).to.bignumber.eq(1);
+        });
+
+        it('abs(int(1)) == int(1)', async () => {
+            const n = 1;
+            const r = await testContract.abs.callAsync(new BigNumber(n));
+            expect(r).to.bignumber.eq(1);
         });
     });
 
@@ -131,6 +160,63 @@ 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('mulDiv(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('mulDiv(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('mulDiv(MIN_FIXED, int(-1), int(1)) throws', async () => {
+            const [a, n, d] = [MIN_FIXED_VALUE, -1, 1];
+            const expectedError = new FixedMathRevertErrors.BinOpError(
+                FixedMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow,
+                a,
+                n,
+            );
+            const tx = testContract.mulDiv.callAsync(a, new BigNumber(n), new BigNumber(d));
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        it('mulDiv(MIN_FIXED, int(-1), int(1)) throws', async () => {
+            const [a, n, d] = [MIN_FIXED_VALUE, -1, 1];
+            const expectedError = new FixedMathRevertErrors.BinOpError(
+                FixedMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow,
+                a,
+                n,
+            );
+            const tx = testContract.mulDiv.callAsync(a, new BigNumber(n), new BigNumber(d));
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        it('mulDiv(MIN_FIXED, int(1), int(-1)) throws', async () => {
+            const [a, n, d] = [MIN_FIXED_VALUE, 1, -1];
+            const expectedError = new FixedMathRevertErrors.BinOpError(
+                FixedMathRevertErrors.BinOpErrorCodes.DivisionOverflow,
+                a,
+                d,
+            );
+            const tx = testContract.mulDiv.callAsync(a, new BigNumber(n), new BigNumber(d));
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        it('mulDiv(MAX_FIXED, int(-1), int(1)) == -MAX_FIXED', async () => {
+            const [a, n, d] = [MAX_FIXED_VALUE, -1, 1];
+            const r = await testContract.mulDiv.callAsync(a, new BigNumber(n), new BigNumber(d));
+            expect(r).to.bignumber.eq(MAX_FIXED_VALUE.negated());
+        });
+
+        it('mulDiv(MAX_FIXED, int(1), int(-1)) == -MAX_FIXED', async () => {
+            const [a, n, d] = [MAX_FIXED_VALUE, 1, -1];
+            const r = await testContract.mulDiv.callAsync(a, new BigNumber(n), new BigNumber(d));
+            expect(r).to.bignumber.eq(MAX_FIXED_VALUE.negated());
+        });
     });
 
     describe('add()', () => {
@@ -170,13 +256,47 @@ 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);
+        });
+
+        it('MAX_FIXED + (MIN_FIXED + int(1)) == 0', async () => {
+            const [a, b] = [MAX_FIXED_VALUE, MIN_FIXED_VALUE.plus(1)];
+            const r = await testContract.add.callAsync(a, b);
+            expect(r).to.bignumber.eq(0);
+        });
     });
 
     describe('sub()', () => {
@@ -205,7 +325,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 +343,45 @@ 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.SignedValueError(
+                FixedMathRevertErrors.ValueErrorCodes.TooSmall,
+                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);
+            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()', () => {
@@ -285,6 +444,73 @@ blockchainTests('LibFixedMath unit tests', env => {
             const tx = testContract.mul.callAsync(a, b);
             return expect(tx).to.revertWith(expectedError);
         });
+
+        it('MAX_FIXED * int(1) == MAX_FIXED / FIXED_1', async () => {
+            const [a, b] = [MAX_FIXED_VALUE, 1];
+            const r = await testContract.mul.callAsync(a, new BigNumber(b));
+            expect(r).to.bignumber.eq(MAX_FIXED_VALUE.dividedToIntegerBy(FIXED_1));
+        });
+
+        it('MAX_FIXED * int(2) throws', async () => {
+            const [a, b] = [MAX_FIXED_VALUE, 2];
+            const expectedError = new FixedMathRevertErrors.BinOpError(
+                FixedMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow,
+                a,
+                b,
+            );
+            const tx = testContract.mul.callAsync(a, new BigNumber(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.MultiplicationOverflow,
+                a,
+                b,
+            );
+            const tx = testContract.mul.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.MultiplicationOverflow,
+                a,
+                b,
+            );
+            const tx = testContract.mul.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.BinOpError(
+                FixedMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow,
+                a,
+                b,
+            );
+            const tx = testContract.mul.callAsync(a, b);
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        it('MIN_FIXED * int(-1) throws', async () => {
+            const [a, b] = [MIN_FIXED_VALUE, -1];
+            const expectedError = new FixedMathRevertErrors.BinOpError(
+                FixedMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow,
+                a,
+                b,
+            );
+            const tx = testContract.mul.callAsync(a, new BigNumber(b));
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        it('MAX_FIXED * int(-1) == -MAX_FIXED / FIXED_1', async () => {
+            const [a, b] = [MAX_FIXED_VALUE, -1];
+            const r = await testContract.mul.callAsync(a, new BigNumber(b));
+            expect(r).to.bignumber.eq(MAX_FIXED_VALUE.negated().dividedToIntegerBy(FIXED_1));
+        });
     });
 
     describe('div()', () => {
@@ -330,6 +556,34 @@ blockchainTests('LibFixedMath unit tests', env => {
             const r = await testContract.div.callAsync(toFixed(a), toFixed(b));
             assertFixedEquals(r, div(a, b));
         });
+
+        it('MIN_FIXED / int(-1) throws', async () => {
+            const [a, b] = [MIN_FIXED_VALUE, -1];
+            const expectedError = new FixedMathRevertErrors.BinOpError(
+                FixedMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow,
+                a,
+                FIXED_1,
+            );
+            const tx = testContract.div.callAsync(a, new BigNumber(b));
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        it('MAX_FIXED / int(-1) throws', async () => {
+            const [a, b] = [MIN_FIXED_VALUE, -1];
+            const expectedError = new FixedMathRevertErrors.BinOpError(
+                FixedMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow,
+                a,
+                FIXED_1,
+            );
+            const tx = testContract.div.callAsync(a, new BigNumber(b));
+            return expect(tx).to.revertWith(expectedError);
+        });
+
+        it('int(-1) / MIN_FIXED == 0', async () => {
+            const [a, b] = [-1, MIN_FIXED_VALUE];
+            const r = await testContract.div.callAsync(new BigNumber(a), b);
+            expect(r).to.bignumber.eq(0);
+        });
     });
 
     describe('uintMul()', () => {

packages/utils/CHANGELOG.json

@@ -1,4 +1,13 @@
 [
+    {
+        "version": "4.6.0-beta.1",
+        "changes": [
+            {
+                "note": "Consolidated FixedMathRevertErrors",
+                "pr": 2255
+            }
+        ]
+    },
     {
         "version": "4.6.0-beta.0",
         "changes": [

packages/utils/src/fixed_math_revert_errors.ts

@@ -10,9 +10,9 @@ export enum ValueErrorCodes {
 
 export enum BinOpErrorCodes {
     AdditionOverflow,
-    SubtractionUnderflow,
     MultiplicationOverflow,
     DivisionByZero,
+    DivisionOverflow,
 }
 
 export class SignedValueError extends RevertError {