From 33e41dd500960fde6bf1f5b1f4cf650731086963 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Thu, 29 Nov 2018 13:56:39 -0800 Subject: More tests + require instead of revert in compliance contract --- .../CompliantForwarder/CompliantForwarder.sol | 19 +++-- .../test/extensions/compliant_forwarder.ts | 89 +++++++++++++++++++--- 2 files changed, 90 insertions(+), 18 deletions(-) (limited to 'packages/contracts') diff --git a/packages/contracts/contracts/extensions/CompliantForwarder/CompliantForwarder.sol b/packages/contracts/contracts/extensions/CompliantForwarder/CompliantForwarder.sol index 2febc5cce..f34ee699d 100644 --- a/packages/contracts/contracts/extensions/CompliantForwarder/CompliantForwarder.sol +++ b/packages/contracts/contracts/extensions/CompliantForwarder/CompliantForwarder.sol @@ -51,8 +51,14 @@ contract CompliantForwarder { if (selector != EXCHANGE_FILL_ORDER_SELECTOR) { revert("EXCHANGE_TRANSACTION_NOT_FILL_ORDER"); } + + // Taker must be compliant + require( + COMPLIANCE_TOKEN.balanceOf(signerAddress) > 0, + "TAKER_UNVERIFIED" + ); - // Extract maker address from fill order transaction + // Extract maker address from fill order transaction and ensure maker is compliant // Below is the table of calldata offsets into a fillOrder transaction. /** ### parameters @@ -82,13 +88,10 @@ contract CompliantForwarder { // Add 0xc to the makerAddress since abi-encoded addresses are left padded with 12 bytes. // Putting this together: makerAddress = 0x60 + 0x4 + 0xc = 0x70 address makerAddress = signedFillOrderTransaction.readAddress(0x70); - - // Verify maker/taker have been verified by the compliance token. - if (COMPLIANCE_TOKEN.balanceOf(makerAddress) == 0) { - revert("MAKER_UNVERIFIED"); - } else if (COMPLIANCE_TOKEN.balanceOf(signerAddress) == 0) { - revert("TAKER_UNVERIFIED"); - } + require( + COMPLIANCE_TOKEN.balanceOf(makerAddress) > 0, + "MAKER_UNVERIFIED" + ); // All entities are verified. Execute fillOrder. EXCHANGE.executeTransaction( diff --git a/packages/contracts/test/extensions/compliant_forwarder.ts b/packages/contracts/test/extensions/compliant_forwarder.ts index 932012c0d..311ad78e9 100644 --- a/packages/contracts/test/extensions/compliant_forwarder.ts +++ b/packages/contracts/test/extensions/compliant_forwarder.ts @@ -4,6 +4,7 @@ import { RevertReason, SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; import { Web3Wrapper } from '@0x/web3-wrapper'; import * as chai from 'chai'; +import * as ethUtil from 'ethereumjs-util'; import { DummyERC20TokenContract } from '../../generated-wrappers/dummy_erc20_token'; import { ExchangeContract } from '../../generated-wrappers/exchange'; @@ -12,9 +13,8 @@ import { YesComplianceTokenContract } from '../../generated-wrappers/yes_complia import { artifacts } from '../../src/artifacts'; import { - expectContractCreationFailedAsync, expectTransactionFailedAsync, - sendTransactionResult, + expectTransactionFailedWithoutReasonAsync, } from '../utils/assertions'; import { chaiSetup } from '../utils/chai_setup'; import { constants } from '../utils/constants'; @@ -36,17 +36,19 @@ describe.only(ContractName.CompliantForwarder, () => { let owner: string; let compliantTakerAddress: string; let feeRecipientAddress: string; - let noncompliantAddress: string; + let nonCompliantAddress: string; let defaultMakerAssetAddress: string; let defaultTakerAssetAddress: string; let zrxAssetData: string; let zrxToken: DummyERC20TokenContract; + let exchangeInstance: ExchangeContract; let exchangeWrapper: ExchangeWrapper; let orderFactory: OrderFactory; let erc20Wrapper: ERC20Wrapper; let erc20Balances: ERC20BalancesByOwner; + let takerTransactionFactory: TransactionFactory; let compliantSignedOrder: SignedOrder; let compliantSignedFillOrderTx: SignedTransaction; let noncompliantSignedFillOrderTx: SignedTransaction; @@ -66,7 +68,7 @@ describe.only(ContractName.CompliantForwarder, () => { compliantMakerAddress, compliantTakerAddress, feeRecipientAddress, - noncompliantAddress, + nonCompliantAddress, ] = accounts); // Create wrappers erc20Wrapper = new ERC20Wrapper(provider, usedAddresses, owner); @@ -91,7 +93,7 @@ describe.only(ContractName.CompliantForwarder, () => { const erc20Proxy = await erc20Wrapper.deployProxyAsync(); await erc20Wrapper.setBalancesAndAllowancesAsync(); // Deploy Exchange congtract - const exchangeInstance = await ExchangeContract.deployFrom0xArtifactAsync( + exchangeInstance = await ExchangeContract.deployFrom0xArtifactAsync( artifacts.Exchange, provider, txDefaults, @@ -164,7 +166,7 @@ describe.only(ContractName.CompliantForwarder, () => { ); // Create Valid/Invalid orders const takerPrivateKey = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(compliantTakerAddress)]; - const takerTransactionFactory = new TransactionFactory(takerPrivateKey, exchangeInstance.address); + takerTransactionFactory = new TransactionFactory(takerPrivateKey, exchangeInstance.address); compliantSignedOrder = await orderFactory.newSignedOrderAsync({ senderAddress: compliantForwarderInstance.address, }); @@ -190,8 +192,7 @@ describe.only(ContractName.CompliantForwarder, () => { beforeEach(async () => { erc20Balances = await erc20Wrapper.getBalancesAsync(); }); - - it.only('should transfer the correct amounts when maker and taker are verified', async () => { + it('should transfer the correct amounts when maker and taker are compliant', async () => { await compliantForwarderInstance.fillOrder.sendTransactionAsync( compliantSignedFillOrderTx.salt, compliantSignedFillOrderTx.signerAddress, @@ -230,8 +231,76 @@ describe.only(ContractName.CompliantForwarder, () => { erc20Balances[feeRecipientAddress][zrxToken.address].add(makerFeePaid.add(takerFeePaid)), ); }); - // @TODO: Should fail if order's senderAddress is not set to the compliant forwarding contract - // @TODO: Should fail if the signed transaction is not intended for fillOrder + it('should revert if the signed transaction is not intended for fillOrder', async () => { + // Create signed order without the fillOrder function selector + const txDataBuf = ethUtil.toBuffer(compliantSignedFillOrderTx.data); + const selectorLengthInBytes = 4; + const txDataBufMinusSelector = txDataBuf.slice(selectorLengthInBytes); + const badSelector = '0x00000000'; + const badSelectorBuf = ethUtil.toBuffer(badSelector); + const txDataBufWithBadSelector = Buffer.concat([badSelectorBuf, txDataBufMinusSelector]); + const txDataBufWithBadSelectorHex = ethUtil.bufferToHex(txDataBufWithBadSelector); + // Call compliant forwarder + return expectTransactionFailedWithoutReasonAsync(compliantForwarderInstance.fillOrder.sendTransactionAsync( + compliantSignedFillOrderTx.salt, + compliantSignedFillOrderTx.signerAddress, + txDataBufWithBadSelectorHex, + compliantSignedFillOrderTx.signature, + )); + }); + it('should revert if senderAddress is not set to the compliant forwarding contract', async () => { + // Create signed order with incorrect senderAddress + const notCompliantForwarderAddress = zrxToken.address; + const signedOrderWithBadSenderAddress = await orderFactory.newSignedOrderAsync({ + senderAddress: notCompliantForwarderAddress, + }); + const signedOrderWithoutExchangeAddress = orderUtils.getOrderWithoutExchangeAddress( + signedOrderWithBadSenderAddress, + ); + const signedOrderWithoutExchangeAddressData = exchangeInstance.fillOrder.getABIEncodedTransactionData( + signedOrderWithoutExchangeAddress, + takerAssetFillAmount, + compliantSignedOrder.signature, + ); + const signedFillOrderTx = takerTransactionFactory.newSignedTransaction( + signedOrderWithoutExchangeAddressData, + ); + // Call compliant forwarder + return expectTransactionFailedWithoutReasonAsync(compliantForwarderInstance.fillOrder.sendTransactionAsync( + signedFillOrderTx.salt, + signedFillOrderTx.signerAddress, + signedFillOrderTx.data, + signedFillOrderTx.signature, + )); + }); + it('should revert if maker address is not compliant (does not hold a Yes Token)', async () => { + // Create signed order with non-compliant maker address + const signedOrderWithBadSenderAddress = await orderFactory.newSignedOrderAsync({ + senderAddress: compliantForwarderInstance.address, + makerAddress: nonCompliantAddress + }); + const signedOrderWithoutExchangeAddress = orderUtils.getOrderWithoutExchangeAddress( + signedOrderWithBadSenderAddress, + ); + const signedOrderWithoutExchangeAddressData = exchangeInstance.fillOrder.getABIEncodedTransactionData( + signedOrderWithoutExchangeAddress, + takerAssetFillAmount, + compliantSignedOrder.signature, + ); + const signedFillOrderTx = takerTransactionFactory.newSignedTransaction( + signedOrderWithoutExchangeAddressData, + ); + // Call compliant forwarder + return expectTransactionFailedAsync( + compliantForwarderInstance.fillOrder.sendTransactionAsync( + signedFillOrderTx.salt, + signedFillOrderTx.signerAddress, + signedFillOrderTx.data, + signedFillOrderTx.signature, + ), + RevertReason.MakerUnverified + ); + }); // @TODO: Should fail if maker is not verified // @TODO: Should fail it taker is not verified }); -- cgit v1.2.3