From 7d5a23969d5514ca3c942f11a48a8e858b9d0188 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 22 Aug 2018 18:01:45 -0700 Subject: Add reentrancy tests for fillOrder and wrapper functions --- packages/contracts/test/exchange/core.ts | 28 ++++ packages/contracts/test/exchange/wrapper.ts | 193 ++++++++++++++++++++++++++++ packages/contracts/test/utils/artifacts.ts | 2 + packages/contracts/test/utils/constants.ts | 16 +++ 4 files changed, 239 insertions(+) (limited to 'packages') diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts index f9d8b7783..8ba55a0ab 100644 --- a/packages/contracts/test/exchange/core.ts +++ b/packages/contracts/test/exchange/core.ts @@ -14,6 +14,7 @@ import { DummyNoReturnERC20TokenContract } from '../../generated_contract_wrappe import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ExchangeCancelEventArgs, ExchangeContract } from '../../generated_contract_wrappers/exchange'; +import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token'; import { TestStaticCallReceiverContract } from '../../generated_contract_wrappers/test_static_call_receiver'; import { artifacts } from '../utils/artifacts'; import { expectTransactionFailedAsync } from '../utils/assertions'; @@ -42,6 +43,7 @@ describe('Exchange core', () => { let zrxToken: DummyERC20TokenContract; let erc721Token: DummyERC721TokenContract; let noReturnErc20Token: DummyNoReturnERC20TokenContract; + let reentrantErc20Token: ReentrantERC20TokenContract; let exchange: ExchangeContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; @@ -117,6 +119,12 @@ describe('Exchange core', () => { provider, txDefaults, ); + reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync( + artifacts.ReentrantERC20Token, + provider, + txDefaults, + exchange.address, + ); defaultMakerAssetAddress = erc20TokenA.address; defaultTakerAssetAddress = erc20TokenB.address; @@ -144,6 +152,26 @@ describe('Exchange core', () => { signedOrder = await orderFactory.newSignedOrderAsync(); }); + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow fillOrder to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('fillOrder reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should throw if signature is invalid', async () => { signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), diff --git a/packages/contracts/test/exchange/wrapper.ts b/packages/contracts/test/exchange/wrapper.ts index d48441dca..aadb5ab59 100644 --- a/packages/contracts/test/exchange/wrapper.ts +++ b/packages/contracts/test/exchange/wrapper.ts @@ -11,6 +11,7 @@ import { DummyERC721TokenContract } from '../../generated_contract_wrappers/dumm import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ExchangeContract } from '../../generated_contract_wrappers/exchange'; +import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token'; import { artifacts } from '../utils/artifacts'; import { expectTransactionFailedAsync } from '../utils/assertions'; import { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from '../utils/block_timestamp'; @@ -40,6 +41,7 @@ describe('Exchange wrappers', () => { let exchange: ExchangeContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; + let reentrantErc20Token: ReentrantERC20TokenContract; let exchangeWrapper: ExchangeWrapper; let erc20Wrapper: ERC20Wrapper; @@ -104,6 +106,13 @@ describe('Exchange wrappers', () => { constants.AWAIT_TRANSACTION_MINED_MS, ); + reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync( + artifacts.ReentrantERC20Token, + provider, + txDefaults, + exchange.address, + ); + defaultMakerAssetAddress = erc20TokenA.address; defaultTakerAssetAddress = erc20TokenB.address; @@ -126,6 +135,26 @@ describe('Exchange wrappers', () => { await blockchainLifecycle.revertAsync(); }); describe('fillOrKillOrder', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow fillOrKillOrder to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('fillOrKillOrder reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), @@ -197,6 +226,25 @@ describe('Exchange wrappers', () => { }); describe('fillOrderNoThrow', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow fillOrderNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.fillOrderNoThrowAsync(signedOrder, takerAddress); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('fillOrderNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const signedOrder = await orderFactory.newSignedOrderAsync({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), @@ -397,6 +445,26 @@ describe('Exchange wrappers', () => { }); describe('batchFillOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchFillOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.batchFillOrdersAsync([signedOrder], takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('batchFillOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; @@ -446,6 +514,26 @@ describe('Exchange wrappers', () => { }); describe('batchFillOrKillOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchFillOrKillOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.batchFillOrKillOrdersAsync([signedOrder], takerAddress), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('batchFillOrKillOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; @@ -512,6 +600,25 @@ describe('Exchange wrappers', () => { }); describe('batchFillOrdersNoThrow', async () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow batchFillOrdersNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.batchFillOrdersNoThrowAsync([signedOrder], takerAddress); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('batchFillOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should transfer the correct amounts', async () => { const takerAssetFillAmounts: BigNumber[] = []; const makerAssetAddress = erc20TokenA.address; @@ -625,6 +732,28 @@ describe('Exchange wrappers', () => { }); describe('marketSellOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketSellOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.marketSellOrdersAsync([signedOrder], takerAddress, { + takerAssetFillAmount: signedOrder.takerAssetAmount, + }), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('marketSellOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire takerAssetFillAmount is filled', async () => { const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus( signedOrders[1].takerAssetAmount.div(2), @@ -717,6 +846,27 @@ describe('Exchange wrappers', () => { }); describe('marketSellOrdersNoThrow', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketSellOrdersNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.marketSellOrdersNoThrowAsync([signedOrder], takerAddress, { + takerAssetFillAmount: signedOrder.takerAssetAmount, + }); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('marketSellOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire takerAssetFillAmount is filled', async () => { const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus( signedOrders[1].takerAssetAmount.div(2), @@ -843,6 +993,28 @@ describe('Exchange wrappers', () => { }); describe('marketBuyOrders', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketBuyOrders to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await expectTransactionFailedAsync( + exchangeWrapper.marketBuyOrdersAsync([signedOrder], takerAddress, { + makerAssetFillAmount: signedOrder.makerAssetAmount, + }), + RevertReason.TransferFailed, + ); + }); + }); + }; + describe('marketBuyOrders reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire makerAssetFillAmount is filled', async () => { const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus( signedOrders[1].makerAssetAmount.div(2), @@ -933,6 +1105,27 @@ describe('Exchange wrappers', () => { }); describe('marketBuyOrdersNoThrow', () => { + const reentrancyTest = (functionNames: string[]) => { + _.forEach(functionNames, async (functionName: string, functionId: number) => { + const description = `should not allow marketBuyOrdersNoThrow to reenter the Exchange contract via ${functionName}`; + it(description, async () => { + const signedOrder = await orderFactory.newSignedOrderAsync({ + makerAssetData: assetDataUtils.encodeERC20AssetData(reentrantErc20Token.address), + }); + await web3Wrapper.awaitTransactionSuccessAsync( + await reentrantErc20Token.setCurrentFunction.sendTransactionAsync(functionId), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await exchangeWrapper.marketBuyOrdersNoThrowAsync([signedOrder], takerAddress, { + makerAssetFillAmount: signedOrder.makerAssetAmount, + }); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(erc20Balances).to.deep.equal(newBalances); + }); + }); + }; + describe('marketBuyOrdersNoThrow reentrancy tests', () => reentrancyTest(constants.FUNCTIONS_WITH_MUTEX)); + it('should stop when the entire makerAssetFillAmount is filled', async () => { const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus( signedOrders[1].makerAssetAmount.div(2), diff --git a/packages/contracts/test/utils/artifacts.ts b/packages/contracts/test/utils/artifacts.ts index 2f6fcef71..5ddb5cc7f 100644 --- a/packages/contracts/test/utils/artifacts.ts +++ b/packages/contracts/test/utils/artifacts.ts @@ -16,6 +16,7 @@ import * as MixinAuthorizable from '../../artifacts/MixinAuthorizable.json'; import * as MultiSigWallet from '../../artifacts/MultiSigWallet.json'; import * as MultiSigWalletWithTimeLock from '../../artifacts/MultiSigWalletWithTimeLock.json'; import * as OrderValidator from '../../artifacts/OrderValidator.json'; +import * as ReentrantERC20Token from '../../artifacts/ReentrantERC20Token.json'; import * as TestAssetProxyDispatcher from '../../artifacts/TestAssetProxyDispatcher.json'; import * as TestAssetProxyOwner from '../../artifacts/TestAssetProxyOwner.json'; import * as TestConstants from '../../artifacts/TestConstants.json'; @@ -49,6 +50,7 @@ export const artifacts = { MultiSigWallet: (MultiSigWallet as any) as ContractArtifact, MultiSigWalletWithTimeLock: (MultiSigWalletWithTimeLock as any) as ContractArtifact, OrderValidator: (OrderValidator as any) as ContractArtifact, + ReentrantERC20Token: (ReentrantERC20Token as any) as ContractArtifact, TestAssetProxyOwner: (TestAssetProxyOwner as any) as ContractArtifact, TestAssetProxyDispatcher: (TestAssetProxyDispatcher as any) as ContractArtifact, TestConstants: (TestConstants as any) as ContractArtifact, diff --git a/packages/contracts/test/utils/constants.ts b/packages/contracts/test/utils/constants.ts index 65eaee398..7060b5091 100644 --- a/packages/contracts/test/utils/constants.ts +++ b/packages/contracts/test/utils/constants.ts @@ -51,4 +51,20 @@ export const constants = { WORD_LENGTH: 32, ZERO_AMOUNT: new BigNumber(0), PERCENTAGE_DENOMINATOR: new BigNumber(10).pow(18), + FUNCTIONS_WITH_MUTEX: [ + 'FILL_ORDER', + 'FILL_OR_KILL_ORDER', + 'FILL_ORDER_NO_THROW', + 'BATCH_FILL_ORDERS', + 'BATCH_FILL_OR_KILL_ORDERS', + 'BATCH_FILL_ORDERS_NO_THROW', + 'MARKET_BUY_ORDERS', + 'MARKET_BUY_ORDERS_NO_THROW', + 'MARKET_SELL_ORDERS', + 'MARKET_SELL_ORDERS_NO_THROW', + 'MATCH_ORDERS', + 'CANCEL_ORDER', + 'CANCEL_ORDERS_UP_TO', + 'SET_SIGNATURE_VALIDATOR_APPROVAL', + ], }; -- cgit