@0x/contracts-zero-ex: Address audit feedback.

This commit is contained in:
Lawrence Forman
2020-06-04 16:15:38 -04:00
parent cb2cc05cac
commit ebfa62637e
26 changed files with 258 additions and 321 deletions

View File

@@ -8,10 +8,9 @@ import {
randomAddress,
verifyEventsFromLogs,
} from '@0x/contracts-test-utils';
import { AbiEncoder, hexUtils, ZeroExRevertErrors } from '@0x/utils';
import { AbiEncoder, hexUtils, OwnableRevertErrors, ZeroExRevertErrors } from '@0x/utils';
import { ETH_TOKEN_ADDRESS } from '../../src/constants';
import { getRLPEncodedAccountNonceAsync } from '../../src/nonce_utils';
import { artifacts } from '../artifacts';
import { abis } from '../utils/abis';
import { fullMigrateAsync } from '../utils/migration';
@@ -27,6 +26,7 @@ import {
} from '../wrappers';
blockchainTests.resets('TransformERC20 feature', env => {
let owner: string;
let taker: string;
let transformerDeployer: string;
let zeroEx: ZeroExContract;
@@ -35,17 +35,21 @@ blockchainTests.resets('TransformERC20 feature', env => {
let allowanceTarget: string;
before(async () => {
let owner;
[owner, taker, transformerDeployer] = await env.getAccountAddressesAsync();
zeroEx = await fullMigrateAsync(owner, env.provider, env.txDefaults, {
transformERC20: await TransformERC20Contract.deployFrom0xArtifactAsync(
artifacts.TestTransformERC20,
env.provider,
env.txDefaults,
artifacts,
transformerDeployer,
),
});
zeroEx = await fullMigrateAsync(
owner,
env.provider,
env.txDefaults,
{
transformERC20: await TransformERC20Contract.deployFrom0xArtifactAsync(
artifacts.TestTransformERC20,
env.provider,
env.txDefaults,
artifacts,
),
},
{ transformerDeployer },
);
feature = new TransformERC20Contract(zeroEx.address, env.provider, env.txDefaults, abis);
wallet = new FlashWalletContract(await feature.getTransformWallet().callAsync(), env.provider, env.txDefaults);
allowanceTarget = await new ITokenSpenderContract(zeroEx.address, env.provider, env.txDefaults)
@@ -53,7 +57,7 @@ blockchainTests.resets('TransformERC20 feature', env => {
.callAsync();
});
const { MAX_UINT256, NULL_BYTES, ZERO_AMOUNT } = constants;
const { MAX_UINT256, ZERO_AMOUNT } = constants;
describe('wallets', () => {
it('createTransformWallet() replaces the current wallet', async () => {
@@ -64,11 +68,39 @@ blockchainTests.resets('TransformERC20 feature', env => {
});
});
describe('transformer deployer', () => {
it('`getTransformerDeployer()` returns the transformer deployer', async () => {
const actualDeployer = await feature.getTransformerDeployer().callAsync();
expect(actualDeployer).to.eq(transformerDeployer);
});
it('owner can set the transformer deployer with `setTransformerDeployer()`', async () => {
const newDeployer = randomAddress();
const receipt = await feature
.setTransformerDeployer(newDeployer)
.awaitTransactionSuccessAsync({ from: owner });
verifyEventsFromLogs(
receipt.logs,
[{ transformerDeployer: newDeployer }],
TransformERC20Events.TransformerDeployerUpdated,
);
const actualDeployer = await feature.getTransformerDeployer().callAsync();
expect(actualDeployer).to.eq(newDeployer);
});
it('non-owner cannot set the transformer deployer with `setTransformerDeployer()`', async () => {
const newDeployer = randomAddress();
const notOwner = randomAddress();
const tx = feature.setTransformerDeployer(newDeployer).callAsync({ from: notOwner });
return expect(tx).to.revertWith(new OwnableRevertErrors.OnlyOwnerError(notOwner, owner));
});
});
describe('_transformERC20()', () => {
let inputToken: TestMintableERC20TokenContract;
let outputToken: TestMintableERC20TokenContract;
let mintTransformer: TestMintTokenERC20TransformerContract;
let rlpNonce: string;
let transformerNonce: number;
before(async () => {
inputToken = await TestMintableERC20TokenContract.deployFrom0xArtifactAsync(
@@ -83,7 +115,7 @@ blockchainTests.resets('TransformERC20 feature', env => {
env.txDefaults,
artifacts,
);
rlpNonce = await getRLPEncodedAccountNonceAsync(env.web3Wrapper, transformerDeployer);
transformerNonce = await env.web3Wrapper.getAccountNonceAsync(transformerDeployer);
mintTransformer = await TestMintTokenERC20TransformerContract.deployFrom0xArtifactAsync(
artifacts.TestMintTokenERC20Transformer,
env.provider,
@@ -97,7 +129,7 @@ blockchainTests.resets('TransformERC20 feature', env => {
});
interface Transformation {
transformer: string;
deploymentNonce: number;
data: string;
}
@@ -111,7 +143,6 @@ blockchainTests.resets('TransformERC20 feature', env => {
{ name: 'burnAmount', type: 'uint256' },
{ name: 'mintAmount', type: 'uint256' },
{ name: 'feeAmount', type: 'uint256' },
{ name: 'deploymentNonce', type: 'bytes' },
],
},
]);
@@ -124,21 +155,21 @@ blockchainTests.resets('TransformERC20 feature', env => {
inputTokenBurnAmunt: Numberish;
outputTokenMintAmount: Numberish;
outputTokenFeeAmount: Numberish;
rlpNonce: string;
deploymentNonce: number;
}> = {},
): Transformation {
const _opts = {
rlpNonce,
outputTokenAddress: outputToken.address,
inputTokenAddress: inputToken.address,
inputTokenBurnAmunt: ZERO_AMOUNT,
outputTokenMintAmount: ZERO_AMOUNT,
outputTokenFeeAmount: ZERO_AMOUNT,
transformer: mintTransformer.address,
deploymentNonce: transformerNonce,
...opts,
};
return {
transformer: _opts.transformer,
deploymentNonce: _opts.deploymentNonce,
data: transformDataEncoder.encode([
{
inputToken: _opts.inputTokenAddress,
@@ -146,7 +177,6 @@ blockchainTests.resets('TransformERC20 feature', env => {
burnAmount: _opts.inputTokenBurnAmunt,
mintAmount: _opts.outputTokenMintAmount,
feeAmount: _opts.outputTokenFeeAmount,
deploymentNonce: _opts.rlpNonce,
},
]),
};
@@ -443,7 +473,7 @@ blockchainTests.resets('TransformERC20 feature', env => {
);
});
it('fails with third-party transformer', async () => {
it('fails with invalid transformer nonce', async () => {
const startingOutputTokenBalance = getRandomInteger(0, '100e18');
const startingInputTokenBalance = getRandomInteger(2, '100e18');
await outputToken.mint(taker, startingOutputTokenBalance).awaitTransactionSuccessAsync();
@@ -452,32 +482,7 @@ blockchainTests.resets('TransformERC20 feature', env => {
const minOutputTokenAmount = getRandomInteger(2, '1e18');
const callValue = getRandomInteger(1, '1e18');
const callDataHash = hexUtils.random();
const transformations = [createMintTokenTransformation({ transformer: randomAddress() })];
const tx = feature
._transformERC20(
callDataHash,
taker,
inputToken.address,
outputToken.address,
inputTokenAmount,
minOutputTokenAmount,
transformations,
)
.awaitTransactionSuccessAsync({ value: callValue });
return expect(tx).to.revertWith(new ZeroExRevertErrors.TransformERC20.InvalidRLPNonceError(NULL_BYTES));
});
it('fails with incorrect transformer RLP nonce', async () => {
const startingOutputTokenBalance = getRandomInteger(0, '100e18');
const startingInputTokenBalance = getRandomInteger(2, '100e18');
await outputToken.mint(taker, startingOutputTokenBalance).awaitTransactionSuccessAsync();
await inputToken.mint(taker, startingInputTokenBalance).awaitTransactionSuccessAsync();
const inputTokenAmount = getRandomPortion(startingInputTokenBalance);
const minOutputTokenAmount = getRandomInteger(2, '1e18');
const callValue = getRandomInteger(1, '1e18');
const callDataHash = hexUtils.random();
const badRlpNonce = '0x00';
const transformations = [createMintTokenTransformation({ rlpNonce: badRlpNonce })];
const transformations = [createMintTokenTransformation({ deploymentNonce: 1337 })];
const tx = feature
._transformERC20(
callDataHash,
@@ -490,36 +495,12 @@ blockchainTests.resets('TransformERC20 feature', env => {
)
.awaitTransactionSuccessAsync({ value: callValue });
return expect(tx).to.revertWith(
new ZeroExRevertErrors.TransformERC20.UnauthorizedTransformerError(
transformations[0].transformer,
badRlpNonce,
new ZeroExRevertErrors.TransformERC20.TransformerFailedError(
undefined,
transformations[0].data,
constants.NULL_BYTES,
),
);
});
it('fails with invalid transformer RLP nonce', async () => {
const startingOutputTokenBalance = getRandomInteger(0, '100e18');
const startingInputTokenBalance = getRandomInteger(2, '100e18');
await outputToken.mint(taker, startingOutputTokenBalance).awaitTransactionSuccessAsync();
await inputToken.mint(taker, startingInputTokenBalance).awaitTransactionSuccessAsync();
const inputTokenAmount = getRandomPortion(startingInputTokenBalance);
const minOutputTokenAmount = getRandomInteger(2, '1e18');
const callValue = getRandomInteger(1, '1e18');
const callDataHash = hexUtils.random();
const badRlpNonce = '0x010203040506';
const transformations = [createMintTokenTransformation({ rlpNonce: badRlpNonce })];
const tx = feature
._transformERC20(
callDataHash,
taker,
inputToken.address,
outputToken.address,
inputTokenAmount,
minOutputTokenAmount,
transformations,
)
.awaitTransactionSuccessAsync({ value: callValue });
return expect(tx).to.revertWith(new ZeroExRevertErrors.TransformERC20.InvalidRLPNonceError(badRlpNonce));
});
});
});

View File

@@ -23,6 +23,7 @@ blockchainTests.resets('Full migration', env => {
let zeroEx: ZeroExContract;
let features: FullFeatures;
let migrator: TestFullMigrationContract;
const transformerDeployer = randomAddress();
before(async () => {
[owner] = await env.getAccountAddressesAsync();
@@ -34,7 +35,7 @@ blockchainTests.resets('Full migration', env => {
artifacts,
env.txDefaults.from as string,
);
const deployCall = migrator.deploy(owner, toFeatureAdddresses(features));
const deployCall = migrator.deploy(owner, toFeatureAdddresses(features), { transformerDeployer });
zeroEx = new ZeroExContract(await deployCall.callAsync(), env.provider, env.txDefaults);
await deployCall.awaitTransactionSuccessAsync();
});
@@ -52,7 +53,9 @@ blockchainTests.resets('Full migration', env => {
it('Non-deployer cannot call deploy()', async () => {
const notDeployer = randomAddress();
const tx = migrator.deploy(owner, toFeatureAdddresses(features)).callAsync({ from: notDeployer });
const tx = migrator
.deploy(owner, toFeatureAdddresses(features), { transformerDeployer })
.callAsync({ from: notDeployer });
return expect(tx).to.revertWith('FullMigration/INVALID_SENDER');
});
@@ -63,7 +66,13 @@ blockchainTests.resets('Full migration', env => {
},
TransformERC20: {
contractType: ITransformERC20Contract,
fns: ['transformERC20', '_transformERC20', 'createTransformWallet', 'getTransformWallet'],
fns: [
'transformERC20',
'_transformERC20',
'createTransformWallet',
'getTransformWallet',
'setTransformerDeployer',
],
},
};
@@ -162,4 +171,16 @@ blockchainTests.resets('Full migration', env => {
return expect(allowanceTarget.authorized(zeroEx.address).callAsync()).to.become(true);
});
});
describe('TransformERC20', () => {
let feature: ITransformERC20Contract;
before(async () => {
feature = new ITransformERC20Contract(zeroEx.address, env.provider, env.txDefaults);
});
it('has the correct transformer deployer', async () => {
return expect(feature.getTransformerDeployer().callAsync()).to.become(transformerDeployer);
});
});
});

View File

@@ -12,7 +12,6 @@ import { Order } from '@0x/types';
import { BigNumber, hexUtils, ZeroExRevertErrors } from '@0x/utils';
import * as _ from 'lodash';
import { rlpEncodeNonce } from '../../src/nonce_utils';
import {
encodeFillQuoteTransformerData,
FillQuoteTransformerData,
@@ -29,7 +28,6 @@ import {
const { NULL_ADDRESS, NULL_BYTES, MAX_UINT256, ZERO_AMOUNT } = constants;
blockchainTests.resets('FillQuoteTransformer', env => {
const deploymentNonce = _.random(0, 0xffffffff);
let maker: string;
let feeRecipient: string;
let exchange: TestFillQuoteTransformerExchangeContract;
@@ -56,7 +54,6 @@ blockchainTests.resets('FillQuoteTransformer', env => {
env.txDefaults,
artifacts,
exchange.address,
new BigNumber(deploymentNonce),
);
host = await TestFillQuoteTransformerHostContract.deployFrom0xArtifactAsync(
artifacts.TestFillQuoteTransformerHost,
@@ -585,24 +582,6 @@ blockchainTests.resets('FillQuoteTransformer', env => {
makerAssetBalance: qfr.makerAssetBought,
});
});
it('returns the RLP-encoded nonce', async () => {
const orders = _.times(1, () => createOrder());
const signatures = orders.map(() => encodeExchangeBehavior());
const qfr = getExpectedSellQuoteFillResults(orders);
const r = await host
.executeTransform(
transformer.address,
takerToken.address,
qfr.takerAssetSpent,
encodeTransformData({
orders,
signatures,
}),
)
.callAsync({ value: qfr.protocolFeePaid });
expect(r).to.eq(rlpEncodeNonce(deploymentNonce));
});
});
describe('buy quotes', () => {
@@ -881,25 +860,5 @@ blockchainTests.resets('FillQuoteTransformer', env => {
makerAssetBalance: qfr.makerAssetBought,
});
});
it('returns the RLP-encoded nonce', async () => {
const orders = _.times(1, () => createOrder());
const signatures = orders.map(() => encodeExchangeBehavior());
const qfr = getExpectedBuyQuoteFillResults(orders);
const r = await host
.executeTransform(
transformer.address,
takerToken.address,
qfr.takerAssetSpent,
encodeTransformData({
orders,
signatures,
side: FillQuoteTransformerSide.Buy,
fillAmount: qfr.makerAssetBought,
}),
)
.callAsync({ value: qfr.protocolFeePaid });
expect(r).to.eq(rlpEncodeNonce(deploymentNonce));
});
});
});

View File

@@ -3,7 +3,6 @@ import { BigNumber, hexUtils } from '@0x/utils';
import * as _ from 'lodash';
import { ETH_TOKEN_ADDRESS } from '../../src/constants';
import { rlpEncodeNonce } from '../../src/nonce_utils';
import { encodePayTakerTransformerData } from '../../src/transformer_data_encoders';
import { artifacts } from '../artifacts';
import { PayTakerTransformerContract, TestMintableERC20TokenContract, TestTransformerHostContract } from '../wrappers';
@@ -12,7 +11,6 @@ const { MAX_UINT256, ZERO_AMOUNT } = constants;
blockchainTests.resets('PayTakerTransformer', env => {
const taker = randomAddress();
const deploymentNonce = _.random(0, 0xffffffff);
let caller: string;
let token: TestMintableERC20TokenContract;
let transformer: PayTakerTransformerContract;
@@ -31,7 +29,6 @@ blockchainTests.resets('PayTakerTransformer', env => {
env.provider,
env.txDefaults,
artifacts,
new BigNumber(deploymentNonce),
);
host = await TestTransformerHostContract.deployFrom0xArtifactAsync(
artifacts.TestTransformerHost,
@@ -147,16 +144,4 @@ blockchainTests.resets('PayTakerTransformer', env => {
ethBalance: amounts[1].dividedToIntegerBy(2),
});
});
it('returns the RLP-encoded nonce', async () => {
const amounts = _.times(2, () => getRandomInteger(1, '1e18'));
const data = encodePayTakerTransformerData({
amounts,
tokens: [token.address, ETH_TOKEN_ADDRESS],
});
await mintHostTokensAsync(amounts[0]);
await sendEtherAsync(host.address, amounts[1]);
const r = await host.rawExecuteTransform(transformer.address, hexUtils.random(), taker, data).callAsync();
expect(r).to.eq(rlpEncodeNonce(deploymentNonce));
});
});

View File

@@ -1,13 +1,11 @@
import { blockchainTests, constants, expect, randomAddress } from '@0x/contracts-test-utils';
import { BigNumber, ZeroExRevertErrors } from '@0x/utils';
import { ZeroExRevertErrors } from '@0x/utils';
import * as _ from 'lodash';
import { rlpEncodeNonce } from '../../src/nonce_utils';
import { artifacts } from '../artifacts';
import { TestDelegateCallerContract, TestTransformerBaseContract } from '../wrappers';
blockchainTests.resets('Transformer (base)', env => {
const deploymentNonce = _.random(0, 0xffffffff);
let deployer: string;
let delegateCaller: TestDelegateCallerContract;
let transformer: TestTransformerBaseContract;
@@ -28,17 +26,9 @@ blockchainTests.resets('Transformer (base)', env => {
from: deployer,
},
artifacts,
new BigNumber(deploymentNonce),
);
});
describe('_getRLPEncodedDeploymentNonce()', () => {
it('returns the RLP encoded deployment nonce', async () => {
const r = await transformer.getRLPEncodedDeploymentNonce().callAsync();
expect(r).to.eq(rlpEncodeNonce(deploymentNonce));
});
});
describe('die()', () => {
it('cannot be called by non-deployer', async () => {
const notDeployer = randomAddress();

View File

@@ -3,7 +3,6 @@ import { BigNumber, ZeroExRevertErrors } from '@0x/utils';
import * as _ from 'lodash';
import { ETH_TOKEN_ADDRESS } from '../../src/constants';
import { rlpEncodeNonce } from '../../src/nonce_utils';
import { encodeWethTransformerData } from '../../src/transformer_data_encoders';
import { artifacts } from '../artifacts';
import { TestWethContract, TestWethTransformerHostContract, WethTransformerContract } from '../wrappers';
@@ -11,7 +10,6 @@ import { TestWethContract, TestWethTransformerHostContract, WethTransformerContr
const { MAX_UINT256, ZERO_AMOUNT } = constants;
blockchainTests.resets('WethTransformer', env => {
const deploymentNonce = _.random(0, 0xffffffff);
let weth: TestWethContract;
let transformer: WethTransformerContract;
let host: TestWethTransformerHostContract;
@@ -29,7 +27,6 @@ blockchainTests.resets('WethTransformer', env => {
env.txDefaults,
artifacts,
weth.address,
new BigNumber(deploymentNonce),
);
host = await TestWethTransformerHostContract.deployFrom0xArtifactAsync(
artifacts.TestWethTransformerHost,
@@ -152,14 +149,4 @@ blockchainTests.resets('WethTransformer', env => {
wethBalance: amount.dividedToIntegerBy(2),
});
});
it('returns the RLP-encoded nonce', async () => {
const amount = getRandomInteger(1, '1e18');
const data = encodeWethTransformerData({
amount,
token: weth.address,
});
const r = await host.executeTransform(amount, transformer.address, data).callAsync({ value: amount });
expect(r).to.eq(rlpEncodeNonce(deploymentNonce));
});
});

View File

@@ -67,14 +67,13 @@ export interface FullFeatures extends BootstrapFeatures {
}
export interface FullMigrationOpts {
transformDeployer: string;
transformerDeployer: string;
}
export async function deployFullFeaturesAsync(
provider: SupportedProvider,
txDefaults: Partial<TxData>,
features: Partial<FullFeatures> = {},
opts: Partial<FullMigrationOpts> = {},
): Promise<FullFeatures> {
return {
...(await deployBootstrapFeaturesAsync(provider, txDefaults)),
@@ -93,7 +92,6 @@ export async function deployFullFeaturesAsync(
provider,
txDefaults,
artifacts,
opts.transformDeployer || (txDefaults.from as string),
)),
};
}
@@ -105,7 +103,7 @@ export async function fullMigrateAsync(
features: Partial<FullFeatures> = {},
opts: Partial<FullMigrationOpts> = {},
): Promise<ZeroExContract> {
const _features = await deployFullFeaturesAsync(provider, txDefaults, features, opts);
const _features = await deployFullFeaturesAsync(provider, txDefaults, features);
const migrator = await FullMigrationContract.deployFrom0xArtifactAsync(
artifacts.FullMigration,
provider,
@@ -113,7 +111,11 @@ export async function fullMigrateAsync(
artifacts,
txDefaults.from as string,
);
const deployCall = migrator.deploy(owner, toFeatureAdddresses(_features));
const _opts = {
transformerDeployer: txDefaults.from as string,
...opts,
};
const deployCall = migrator.deploy(owner, toFeatureAdddresses(_features), _opts);
const zeroEx = new ZeroExContract(await deployCall.callAsync(), provider, {});
await deployCall.awaitTransactionSuccessAsync();
return zeroEx;