From 063d6ff24e7b3a135e3b232b76ffae3c9a5f6a39 Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Thu, 10 Oct 2019 09:15:00 +0900 Subject: [PATCH] `@0x/contracts-staking`: Add more overflow safeguards to `LibFixedMath`. --- .../contracts/src/libs/LibFixedMath.sol | 13 ++ .../src/libs/LibFixedMathRichErrors.sol | 3 +- .../test/unit_tests/lib_fixed_math_test.ts | 189 +++++++++++++++++- 3 files changed, 193 insertions(+), 12 deletions(-) diff --git a/contracts/staking/contracts/src/libs/LibFixedMath.sol b/contracts/staking/contracts/src/libs/LibFixedMath.sol index dc5ed65802..d5c3e52f76 100644 --- a/contracts/staking/contracts/src/libs/LibFixedMath.sol +++ b/contracts/staking/contracts/src/libs/LibFixedMath.sol @@ -95,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 { @@ -361,6 +367,13 @@ library LibFixedMath { b )); } + if (a == MIN_FIXED_VAL && b == -1) { + LibRichErrors.rrevert(LibFixedMathRichErrors.BinOpError( + LibFixedMathRichErrors.BinOpErrorCodes.DIVISION_OVERFLOW, + a, + b + )); + } c = a / b; } diff --git a/contracts/staking/contracts/src/libs/LibFixedMathRichErrors.sol b/contracts/staking/contracts/src/libs/LibFixedMathRichErrors.sol index 4a4f111fcb..5b310150f4 100644 --- a/contracts/staking/contracts/src/libs/LibFixedMathRichErrors.sol +++ b/contracts/staking/contracts/src/libs/LibFixedMathRichErrors.sol @@ -31,7 +31,8 @@ library LibFixedMathRichErrors { enum BinOpErrorCodes { ADDITION_OVERFLOW, MULTIPLICATION_OVERFLOW, - DIVISION_BY_ZERO + DIVISION_BY_ZERO, + DIVISION_OVERFLOW } // bytes4(keccak256("SignedValueError(uint8,int256)")) 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 eebfeaecbe..e2f2c8f48c 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.only('LibFixedMath unit tests', env => { +blockchainTests('LibFixedMath unit tests', env => { let testContract: TestLibFixedMathContract; before(async () => { @@ -21,6 +21,7 @@ blockchainTests.only('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.only('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); }); }); @@ -132,18 +161,62 @@ blockchainTests.only('LibFixedMath unit tests', env => { return expect(tx).to.revertWith(expectedError); }); - it('int(-1) * int(1) / int(-1) == int(1)', async () => { + 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('-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); + 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()', () => { @@ -269,9 +342,8 @@ blockchainTests.only('LibFixedMath unit tests', env => { 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, + const expectedError = new FixedMathRevertErrors.SignedValueError( + FixedMathRevertErrors.ValueErrorCodes.TooSmall, b, ); const tx = testContract.sub.callAsync(a, b); @@ -281,7 +353,7 @@ blockchainTests.only('LibFixedMath unit tests', env => { 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); + expect(r).to.bignumber.eq(0); }); it('MIN_FIXED - MAX_FIXED throws', async () => { @@ -366,6 +438,73 @@ blockchainTests.only('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()', () => { @@ -411,6 +550,34 @@ blockchainTests.only('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()', () => {