diff --git a/contracts/exchange/test/core.ts b/contracts/exchange/test/core.ts index 11ed9a76f8..fbcdda23e5 100644 --- a/contracts/exchange/test/core.ts +++ b/contracts/exchange/test/core.ts @@ -33,7 +33,7 @@ import { import { BlockchainLifecycle } from '@0x/dev-utils'; import { assetDataUtils, ExchangeRevertErrors, LibMathRevertErrors, orderHashUtils } from '@0x/order-utils'; import { RevertReason, SignatureType, SignedOrder } from '@0x/types'; -import { BigNumber, providerUtils, StringRevertError } from '@0x/utils'; +import { BigNumber, providerUtils, ReentrancyGuardRevertErrors, StringRevertError } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as chai from 'chai'; import { LogWithDecodedArgs } from 'ethereum-types'; @@ -246,8 +246,9 @@ describe('Exchange core', () => { makerAssetData: await assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), }); await reentrantErc20Token.setReentrantFunction.awaitTransactionSuccessAsync(functionId); + const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); const tx = exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); - return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); + return expect(tx).to.revertWith(expectedError); }); }); }; diff --git a/contracts/exchange/test/match_orders.ts b/contracts/exchange/test/match_orders.ts index b2033914d1..78a35f9bcb 100644 --- a/contracts/exchange/test/match_orders.ts +++ b/contracts/exchange/test/match_orders.ts @@ -14,8 +14,8 @@ import { DummyERC721TokenContract } from '@0x/contracts-erc721'; import { chaiSetup, constants, OrderFactory, provider, txDefaults, web3Wrapper } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; import { assetDataUtils, ExchangeRevertErrors, orderHashUtils } from '@0x/order-utils'; -import { OrderStatus, RevertReason } from '@0x/types'; -import { BigNumber, providerUtils } from '@0x/utils'; +import { OrderStatus } from '@0x/types'; +import { BigNumber, providerUtils, ReentrancyGuardRevertErrors } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as chai from 'chai'; import * as _ from 'lodash'; @@ -655,8 +655,9 @@ describe('matchOrders', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); + const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); const tx = exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress); - return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); + return expect(tx).to.revertWith(expectedError); }); }); }; diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index c19ffbf2ba..4c9e5f4556 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/contracts/exchange/test/wrapper.ts @@ -15,8 +15,8 @@ import { } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; import { assetDataUtils, ExchangeRevertErrors, orderHashUtils } from '@0x/order-utils'; -import { OrderStatus, RevertReason, SignedOrder } from '@0x/types'; -import { BigNumber, providerUtils } from '@0x/utils'; +import { OrderStatus, SignedOrder } from '@0x/types'; +import { BigNumber, providerUtils, ReentrancyGuardRevertErrors } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as chai from 'chai'; import { LogWithDecodedArgs } from 'ethereum-types'; @@ -36,7 +36,7 @@ const expect = chai.expect; const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); // tslint:disable:no-unnecessary-type-assertion -describe('Exchange wrappers', () => { +describe.only('Exchange wrappers', () => { let chainId: number; let makerAddress: string; let owner: string; @@ -170,8 +170,9 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); + const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); const tx = exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress); - return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); + return expect(tx).to.revertWith(expectedError); }); } }; @@ -654,8 +655,9 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); + const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); const tx = exchangeWrapper.batchFillOrdersAsync([signedOrder], takerAddress); - return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); + return expect(tx).to.revertWith(expectedError); }); } }; @@ -739,8 +741,9 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); + const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); const tx = exchangeWrapper.batchFillOrKillOrdersAsync([signedOrder], takerAddress); - return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); + return expect(tx).to.revertWith(expectedError); }); } }; @@ -1007,10 +1010,11 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); + const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); const tx = exchangeWrapper.marketSellOrdersAsync([signedOrder], takerAddress, { takerAssetFillAmount: signedOrder.takerAssetAmount, }); - return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); + return expect(tx).to.revertWith(expectedError); }); } }; @@ -1384,10 +1388,11 @@ describe('Exchange wrappers', () => { await reentrantErc20Token.setReentrantFunction.sendTransactionAsync(functionId), constants.AWAIT_TRANSACTION_MINED_MS, ); + const expectedError = new ReentrancyGuardRevertErrors.IllegalReentrancyError(); const tx = exchangeWrapper.marketBuyOrdersAsync([signedOrder], takerAddress, { makerAssetFillAmount: signedOrder.makerAssetAmount, }); - return expect(tx).to.revertWith(RevertReason.ReentrancyIllegal); + return expect(tx).to.revertWith(expectedError); }); } }; diff --git a/contracts/utils/contracts/src/MixinReentrancyGuardRichErrors.sol b/contracts/utils/contracts/src/MixinReentrancyGuardRichErrors.sol new file mode 100644 index 0000000000..de7b7ee64c --- /dev/null +++ b/contracts/utils/contracts/src/MixinReentrancyGuardRichErrors.sol @@ -0,0 +1,39 @@ +/* + + Copyright 2019 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.5.9; + +import "./RichErrors.sol"; + + +contract MixinReentrancyGuardRichErrors is + RichErrors +{ + // bytes4(keccak256("IllegalReentrancyError()")) + bytes internal constant ILLEGAL_REENTRANCY_ERROR_SELECTOR_BYTES = + hex"0c3b823f"; + + // solhint-disable func-name-mixedcase + function IllegalReentrancyError() + internal + pure + returns (bytes memory) + { + return ILLEGAL_REENTRANCY_ERROR_SELECTOR_BYTES; + } +} diff --git a/contracts/utils/contracts/src/ReentrancyGuard.sol b/contracts/utils/contracts/src/ReentrancyGuard.sol index 1fbf12263d..f890023cc9 100644 --- a/contracts/utils/contracts/src/ReentrancyGuard.sol +++ b/contracts/utils/contracts/src/ReentrancyGuard.sol @@ -1,6 +1,6 @@ /* - Copyright 2018 ZeroEx Intl. + Copyright 2019 ZeroEx Intl. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -18,9 +18,12 @@ pragma solidity ^0.5.9; +import "./MixinReentrancyGuardRichErrors.sol"; -contract ReentrancyGuard { +contract ReentrancyGuard is + MixinReentrancyGuardRichErrors +{ // Mutex counter. // Starts at 1 and increases whenever a nonReentrant function is called. uint256 private reentrancyGuardCounter = 1; @@ -31,11 +34,10 @@ contract ReentrancyGuard { uint256 localCounter = ++reentrancyGuardCounter; // Call the function. _; - // If the counter value is different from what we remember, - // the function was called more than once. - require( - localCounter == reentrancyGuardCounter, - "REENTRANCY_ILLEGAL" - ); + // If the counter value is different from what we remember, the function + // was called more than once and an illegal reentrancy occured. + if (localCounter != reentrancyGuardCounter) { + _rrevert(IllegalReentrancyError()); + } } } diff --git a/contracts/utils/test/lib_address_array.ts b/contracts/utils/test/lib_address_array.ts index e88fe892ed..4f594ace4f 100644 --- a/contracts/utils/test/lib_address_array.ts +++ b/contracts/utils/test/lib_address_array.ts @@ -1,10 +1,4 @@ -import { - addressUtils, - chaiSetup, - provider, - txDefaults, - web3Wrapper, -} from '@0x/contracts-test-utils'; +import { addressUtils, chaiSetup, provider, txDefaults, web3Wrapper } from '@0x/contracts-test-utils'; import { BlockchainLifecycle } from '@0x/dev-utils'; import { BigNumber, LibAddressArrayRevertErrors } from '@0x/utils'; import * as chai from 'chai'; diff --git a/contracts/utils/test/lib_bytes.ts b/contracts/utils/test/lib_bytes.ts index 1dfd95fc00..69eaa8f196 100644 --- a/contracts/utils/test/lib_bytes.ts +++ b/contracts/utils/test/lib_bytes.ts @@ -630,7 +630,7 @@ describe('LibBytes', () => { const expectedError = new LibBytesRevertErrors.InvalidByteOperationError( LibBytesRevertErrors.InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsNestedBytesLengthRequired, byteLen, - (new BigNumber(testBytes32)).plus(32), + new BigNumber(testBytes32).plus(32), ); return expect(libBytes.publicReadBytesWithLength.callAsync(testBytes32, offset)).to.revertWith( expectedError, diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index b7b9efcde8..a9c392d577 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -1,6 +1,7 @@ import * as LibAddressArrayRevertErrors from './lib_address_array_revert_errors'; import * as LibBytesRevertErrors from './lib_bytes_revert_errors'; import * as OwnableRevertErrors from './ownable_revert_errors'; +import * as ReentrancyGuardRevertErrors from './reentrancy_guard_revert_errors'; import * as SafeMathRevertErrors from './safe_math_revert_errors'; export { promisify } from './promisify'; @@ -30,4 +31,10 @@ export { AnyRevertError, } from './revert_error'; -export { LibAddressArrayRevertErrors, LibBytesRevertErrors, OwnableRevertErrors, SafeMathRevertErrors }; +export { + LibAddressArrayRevertErrors, + LibBytesRevertErrors, + OwnableRevertErrors, + ReentrancyGuardRevertErrors, + SafeMathRevertErrors, +}; diff --git a/packages/utils/src/lib_address_array_revert_errors.ts b/packages/utils/src/lib_address_array_revert_errors.ts index 22ec284cd9..f80ab28a6f 100644 --- a/packages/utils/src/lib_address_array_revert_errors.ts +++ b/packages/utils/src/lib_address_array_revert_errors.ts @@ -3,16 +3,12 @@ import { RevertError } from './revert_error'; export class MismanagedMemoryError extends RevertError { constructor(freeMemPtr?: BigNumber, addressArrayEndPtr?: BigNumber) { - super( - 'MismanagedMemoryError', - 'MismanagedMemoryError(uint256 freeMemPtr, uint256 addressArrayEndPtr)', - { - freeMemPtr, - addressArrayEndPtr, - }, - ); + super('MismanagedMemoryError', 'MismanagedMemoryError(uint256 freeMemPtr, uint256 addressArrayEndPtr)', { + freeMemPtr, + addressArrayEndPtr, + }); } } -// Register the InvalidByteOperationError type +// Register the MismanagedMemoryError type RevertError.registerType(MismanagedMemoryError); diff --git a/packages/utils/src/reentrancy_guard_revert_errors.ts b/packages/utils/src/reentrancy_guard_revert_errors.ts new file mode 100644 index 0000000000..828dc2bb77 --- /dev/null +++ b/packages/utils/src/reentrancy_guard_revert_errors.ts @@ -0,0 +1,10 @@ +import { RevertError } from './revert_error'; + +export class IllegalReentrancyError extends RevertError { + constructor() { + super('IllegalReentrancyError', 'IllegalReentrancyError()', {}); + } +} + +// Register the IllegalReentrancyError type +RevertError.registerType(IllegalReentrancyError);