More tests + require instead of revert in compliance contract

3 files changed, 92 insertions, 18 deletions

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
     });
diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts
index 6b728af71..0c6fd7fd7 100644
--- a/packages/types/src/index.ts
+++ b/packages/types/src/index.ts
@@ -243,6 +243,8 @@ export enum RevertReason {
     AuctionNotStarted = 'AUCTION_NOT_STARTED',
     AuctionInvalidBeginTime = 'INVALID_BEGIN_TIME',
     InvalidAssetData = 'INVALID_ASSET_DATA',
+    MakerUnverified = 'MAKER_UNVERIFED',
+    TakerUnverified = 'TAKER_UNVERIFIED',
 }
 
 export enum StatusCodes {