crowetic/protocol
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add reentrancy tests for fillOrder and wrapper functions

Browse Source
This commit is contained in:
Amir Bandeali 2018-08-22 18:01:45 -07:00
parent 56c3c29feb
commit 7d5a23969d
4 changed files with 239 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
packages/contracts/test/exchange/core.ts
View File

@ -14,6 +14,7 @@ import { DummyNoReturnERC20TokenContract } from '../../generated_contract_wrappe
import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy';
import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy';
import { ExchangeCancelEventArgs, ExchangeContract } from '../../generated_contract_wrappers/exchange'; 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 { TestStaticCallReceiverContract } from '../../generated_contract_wrappers/test_static_call_receiver';
import { artifacts } from '../utils/artifacts'; import { artifacts } from '../utils/artifacts';
import { expectTransactionFailedAsync } from '../utils/assertions'; import { expectTransactionFailedAsync } from '../utils/assertions';
@ -42,6 +43,7 @@ describe('Exchange core', () => {
let zrxToken: DummyERC20TokenContract; let zrxToken: DummyERC20TokenContract;
let erc721Token: DummyERC721TokenContract; let erc721Token: DummyERC721TokenContract;
let noReturnErc20Token: DummyNoReturnERC20TokenContract; let noReturnErc20Token: DummyNoReturnERC20TokenContract;
let reentrantErc20Token: ReentrantERC20TokenContract;
let exchange: ExchangeContract; let exchange: ExchangeContract;
let erc20Proxy: ERC20ProxyContract; let erc20Proxy: ERC20ProxyContract;
let erc721Proxy: ERC721ProxyContract; let erc721Proxy: ERC721ProxyContract;
@ -117,6 +119,12 @@ describe('Exchange core', () => {
provider, provider,
txDefaults, txDefaults,
); );
reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync(
artifacts.ReentrantERC20Token,
provider,
txDefaults,
exchange.address,
);
defaultMakerAssetAddress = erc20TokenA.address; defaultMakerAssetAddress = erc20TokenA.address;
defaultTakerAssetAddress = erc20TokenB.address; defaultTakerAssetAddress = erc20TokenB.address;
@ -144,6 +152,26 @@ describe('Exchange core', () => {
signedOrder = await orderFactory.newSignedOrderAsync(); 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 () => { it('should throw if signature is invalid', async () => {
signedOrder = await orderFactory.newSignedOrderAsync({ signedOrder = await orderFactory.newSignedOrderAsync({
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(10), 18),

193
packages/contracts/test/exchange/wrapper.ts
View File

@ -11,6 +11,7 @@ import { DummyERC721TokenContract } from '../../generated_contract_wrappers/dumm
import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy';
import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy';
import { ExchangeContract } from '../../generated_contract_wrappers/exchange'; import { ExchangeContract } from '../../generated_contract_wrappers/exchange';
import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token';
import { artifacts } from '../utils/artifacts'; import { artifacts } from '../utils/artifacts';
import { expectTransactionFailedAsync } from '../utils/assertions'; import { expectTransactionFailedAsync } from '../utils/assertions';
import { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from '../utils/block_timestamp'; import { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from '../utils/block_timestamp';
@ -40,6 +41,7 @@ describe('Exchange wrappers', () => {
let exchange: ExchangeContract; let exchange: ExchangeContract;
let erc20Proxy: ERC20ProxyContract; let erc20Proxy: ERC20ProxyContract;
let erc721Proxy: ERC721ProxyContract; let erc721Proxy: ERC721ProxyContract;
let reentrantErc20Token: ReentrantERC20TokenContract;
let exchangeWrapper: ExchangeWrapper; let exchangeWrapper: ExchangeWrapper;
let erc20Wrapper: ERC20Wrapper; let erc20Wrapper: ERC20Wrapper;
@ -104,6 +106,13 @@ describe('Exchange wrappers', () => {
constants.AWAIT_TRANSACTION_MINED_MS, constants.AWAIT_TRANSACTION_MINED_MS,
); );
reentrantErc20Token = await ReentrantERC20TokenContract.deployFrom0xArtifactAsync(
artifacts.ReentrantERC20Token,
provider,
txDefaults,
exchange.address,
);
defaultMakerAssetAddress = erc20TokenA.address; defaultMakerAssetAddress = erc20TokenA.address;
defaultTakerAssetAddress = erc20TokenB.address; defaultTakerAssetAddress = erc20TokenB.address;
@ -126,6 +135,26 @@ describe('Exchange wrappers', () => {
await blockchainLifecycle.revertAsync(); await blockchainLifecycle.revertAsync();
}); });
describe('fillOrKillOrder', () => { 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 () => { it('should transfer the correct amounts', async () => {
const signedOrder = await orderFactory.newSignedOrderAsync({ const signedOrder = await orderFactory.newSignedOrderAsync({
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18),
@ -197,6 +226,25 @@ describe('Exchange wrappers', () => {
}); });
describe('fillOrderNoThrow', () => { 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 () => { it('should transfer the correct amounts', async () => {
const signedOrder = await orderFactory.newSignedOrderAsync({ const signedOrder = await orderFactory.newSignedOrderAsync({
makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18),
@ -397,6 +445,26 @@ describe('Exchange wrappers', () => {
}); });
describe('batchFillOrders', () => { 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 () => { it('should transfer the correct amounts', async () => {
const takerAssetFillAmounts: BigNumber[] = []; const takerAssetFillAmounts: BigNumber[] = [];
const makerAssetAddress = erc20TokenA.address; const makerAssetAddress = erc20TokenA.address;
@ -446,6 +514,26 @@ describe('Exchange wrappers', () => {
}); });
describe('batchFillOrKillOrders', () => { 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 () => { it('should transfer the correct amounts', async () => {
const takerAssetFillAmounts: BigNumber[] = []; const takerAssetFillAmounts: BigNumber[] = [];
const makerAssetAddress = erc20TokenA.address; const makerAssetAddress = erc20TokenA.address;
@ -512,6 +600,25 @@ describe('Exchange wrappers', () => {
}); });
describe('batchFillOrdersNoThrow', async () => { 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 () => { it('should transfer the correct amounts', async () => {
const takerAssetFillAmounts: BigNumber[] = []; const takerAssetFillAmounts: BigNumber[] = [];
const makerAssetAddress = erc20TokenA.address; const makerAssetAddress = erc20TokenA.address;
@ -625,6 +732,28 @@ describe('Exchange wrappers', () => {
}); });
describe('marketSellOrders', () => { 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 () => { it('should stop when the entire takerAssetFillAmount is filled', async () => {
const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus( const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus(
signedOrders[1].takerAssetAmount.div(2), signedOrders[1].takerAssetAmount.div(2),
@ -717,6 +846,27 @@ describe('Exchange wrappers', () => {
}); });
describe('marketSellOrdersNoThrow', () => { 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 () => { it('should stop when the entire takerAssetFillAmount is filled', async () => {
const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus( const takerAssetFillAmount = signedOrders[0].takerAssetAmount.plus(
signedOrders[1].takerAssetAmount.div(2), signedOrders[1].takerAssetAmount.div(2),
@ -843,6 +993,28 @@ describe('Exchange wrappers', () => {
}); });
describe('marketBuyOrders', () => { 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 () => { it('should stop when the entire makerAssetFillAmount is filled', async () => {
const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus( const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus(
signedOrders[1].makerAssetAmount.div(2), signedOrders[1].makerAssetAmount.div(2),
@ -933,6 +1105,27 @@ describe('Exchange wrappers', () => {
}); });
describe('marketBuyOrdersNoThrow', () => { 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 () => { it('should stop when the entire makerAssetFillAmount is filled', async () => {
const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus( const makerAssetFillAmount = signedOrders[0].makerAssetAmount.plus(
signedOrders[1].makerAssetAmount.div(2), signedOrders[1].makerAssetAmount.div(2),

2
packages/contracts/test/utils/artifacts.ts
View File

@ -16,6 +16,7 @@ import * as MixinAuthorizable from '../../artifacts/MixinAuthorizable.json';
import * as MultiSigWallet from '../../artifacts/MultiSigWallet.json'; import * as MultiSigWallet from '../../artifacts/MultiSigWallet.json';
import * as MultiSigWalletWithTimeLock from '../../artifacts/MultiSigWalletWithTimeLock.json'; import * as MultiSigWalletWithTimeLock from '../../artifacts/MultiSigWalletWithTimeLock.json';
import * as OrderValidator from '../../artifacts/OrderValidator.json'; import * as OrderValidator from '../../artifacts/OrderValidator.json';
import * as ReentrantERC20Token from '../../artifacts/ReentrantERC20Token.json';
import * as TestAssetProxyDispatcher from '../../artifacts/TestAssetProxyDispatcher.json'; import * as TestAssetProxyDispatcher from '../../artifacts/TestAssetProxyDispatcher.json';
import * as TestAssetProxyOwner from '../../artifacts/TestAssetProxyOwner.json'; import * as TestAssetProxyOwner from '../../artifacts/TestAssetProxyOwner.json';
import * as TestConstants from '../../artifacts/TestConstants.json'; import * as TestConstants from '../../artifacts/TestConstants.json';
@ -49,6 +50,7 @@ export const artifacts = {
MultiSigWallet: (MultiSigWallet as any) as ContractArtifact, MultiSigWallet: (MultiSigWallet as any) as ContractArtifact,
MultiSigWalletWithTimeLock: (MultiSigWalletWithTimeLock as any) as ContractArtifact, MultiSigWalletWithTimeLock: (MultiSigWalletWithTimeLock as any) as ContractArtifact,
OrderValidator: (OrderValidator as any) as ContractArtifact, OrderValidator: (OrderValidator as any) as ContractArtifact,
ReentrantERC20Token: (ReentrantERC20Token as any) as ContractArtifact,
TestAssetProxyOwner: (TestAssetProxyOwner as any) as ContractArtifact, TestAssetProxyOwner: (TestAssetProxyOwner as any) as ContractArtifact,
TestAssetProxyDispatcher: (TestAssetProxyDispatcher as any) as ContractArtifact, TestAssetProxyDispatcher: (TestAssetProxyDispatcher as any) as ContractArtifact,
TestConstants: (TestConstants as any) as ContractArtifact, TestConstants: (TestConstants as any) as ContractArtifact,

16
packages/contracts/test/utils/constants.ts
View File

@ -51,4 +51,20 @@ export const constants = {
WORD_LENGTH: 32, WORD_LENGTH: 32,
ZERO_AMOUNT: new BigNumber(0), ZERO_AMOUNT: new BigNumber(0),
PERCENTAGE_DENOMINATOR: new BigNumber(10).pow(18), 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',
],
}; };