diff --git a/contracts/staking/CHANGELOG.json b/contracts/staking/CHANGELOG.json index 6ffc8f5be0..51328f4e8c 100644 --- a/contracts/staking/CHANGELOG.json +++ b/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": [ diff --git a/contracts/staking/contracts/src/libs/LibFixedMath.sol b/contracts/staking/contracts/src/libs/LibFixedMath.sol index bab8d2db75..d5c3e52f76 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); } @@ -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) { - 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..5b310150f4 100644 --- a/contracts/staking/contracts/src/libs/LibFixedMathRichErrors.sol +++ b/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)")) 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..6dd3a42fdf 100644 --- a/contracts/staking/test/unit_tests/lib_fixed_math_test.ts +++ b/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()', () => { diff --git a/packages/utils/CHANGELOG.json b/packages/utils/CHANGELOG.json index c9fd40d73f..356ae9535e 100644 --- a/packages/utils/CHANGELOG.json +++ b/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": [ diff --git a/packages/utils/src/fixed_math_revert_errors.ts b/packages/utils/src/fixed_math_revert_errors.ts index b405653e6d..5fb1f3ffc4 100644 --- a/packages/utils/src/fixed_math_revert_errors.ts +++ b/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 {