From 4f56d686895874b3a3cd227e38d13a4ffe3a3c1e Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Fri, 1 Nov 2019 12:51:19 -0400 Subject: [PATCH] `@0x/contracts-staking`: Fix overflow w/ `LibFixedMath._mul(-1, -2*255)`. --- contracts/staking/CHANGELOG.json | 4 ++++ .../staking/contracts/src/libs/LibFixedMath.sol | 2 +- .../test/unit_tests/lib_fixed_math_test.ts | 17 ++++++++++++++--- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/contracts/staking/CHANGELOG.json b/contracts/staking/CHANGELOG.json index f5f39364a7..58226795f7 100644 --- a/contracts/staking/CHANGELOG.json +++ b/contracts/staking/CHANGELOG.json @@ -21,6 +21,10 @@ { "note": "The fallback function in `StakingProxy` reverts if there is no staking contract attached", "pr": 2310 + }, + { + "note": "Fix overflow w/ `LibFixedMath._mul(-1, -2*255)", + "pr": 2311 } ] }, diff --git a/contracts/staking/contracts/src/libs/LibFixedMath.sol b/contracts/staking/contracts/src/libs/LibFixedMath.sol index d5c3e52f76..2d11ce9f46 100644 --- a/contracts/staking/contracts/src/libs/LibFixedMath.sol +++ b/contracts/staking/contracts/src/libs/LibFixedMath.sol @@ -349,7 +349,7 @@ library LibFixedMath { return 0; } c = a * b; - if (c / a != b) { + if (c / a != b || c / b != a) { LibRichErrors.rrevert(LibFixedMathRichErrors.BinOpError( LibFixedMathRichErrors.BinOpErrorCodes.MULTIPLICATION_OVERFLOW, a, 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 6dd3a42fdf..2e98089a67 100644 --- a/contracts/staking/test/unit_tests/lib_fixed_math_test.ts +++ b/contracts/staking/test/unit_tests/lib_fixed_math_test.ts @@ -184,14 +184,14 @@ blockchainTests('LibFixedMath unit tests', env => { 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]; + it('mulDiv(int(-1), MIN_FIXED, int(1)) throws', async () => { + const [a, n, d] = [-1, MIN_FIXED_VALUE, 1]; const expectedError = new FixedMathRevertErrors.BinOpError( FixedMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow, a, n, ); - const tx = testContract.mulDiv.callAsync(a, new BigNumber(n), new BigNumber(d)); + const tx = testContract.mulDiv.callAsync(new BigNumber(a), n, new BigNumber(d)); return expect(tx).to.revertWith(expectedError); }); @@ -506,6 +506,17 @@ blockchainTests('LibFixedMath unit tests', env => { return expect(tx).to.revertWith(expectedError); }); + it('int(-1) * MIN_FIXED throws', async () => { + const [a, b] = [-1, MIN_FIXED_VALUE]; + const expectedError = new FixedMathRevertErrors.BinOpError( + FixedMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow, + a, + b, + ); + const tx = testContract.mul.callAsync(new BigNumber(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));