Update Whitelist contract with comments, also require maker to be whitelisted

5 files changed, 72 insertions, 16 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
index f93a80705..d28df11d8 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol
@@ -47,7 +47,10 @@ contract MixinTransactions is
         external
     {
         // Prevent reentrancy
-        require(currentContextAddress == address(0));
+        require(
+            currentContextAddress == address(0),
+            REENTRANCY_NOT_ALLOWED
+        );
 
         // Calculate transaction hash
         bytes32 transactionHash = keccak256(
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
index 4712ee36c..7d67e5080 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/libs/LibExchangeErrors.sol
@@ -32,6 +32,7 @@ contract LibExchangeErrors {
     string constant INVALID_ORDER_MAKER_ASSET_AMOUNT = "Invalid order maker asset amount: expected a non-zero value.";
 
     // Transaction revert reasons
+    string constant REENTRANCY_NOT_ALLOWED = "`executeTransaction` is not allowed to call itself recursively.";
     string constant DUPLICATE_TRANSACTION_HASH = "Transaction has already been executed.";
     string constant TRANSACTION_EXECUTION_FAILED = "Transaction execution failed.";
 
diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol
index c53856602..4a2e9a931 100644
--- a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol
+++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol
@@ -23,22 +23,30 @@ import "../../protocol/Exchange/interfaces/IExchange.sol";
 import "../../protocol/Exchange/libs/LibOrder.sol";
 import "../../utils/Ownable/Ownable.sol";
 
-contract Whitelist is Ownable {
+contract Whitelist is 
+    Ownable
+{
+    // Revert reasons
+    string constant ADDRESS_NOT_WHITELISTED = "Address not whitelisted.";
 
+    // Mapping of address => whitelist status.
     mapping (address => bool) public isWhitelisted;
+
+    // Exchange contract.
     IExchange EXCHANGE;
 
-    bytes txOriginSignature = new bytes(1);
-    bytes4 fillOrderFunctionSelector;
+    // TxOrigin signature type is the 5th value in enum SignatureType and has a length of 1.
+    bytes constant TX_ORIGIN_SIGNATURE = "\x04";
 
-    function Whitelist(address _exchange)
+    constructor (address _exchange)
         public
     {
         EXCHANGE = IExchange(_exchange);
-        txOriginSignature[0] = 0x04;
-        fillOrderFunctionSelector = EXCHANGE.fillOrder.selector;
     }
 
+    /// @dev Adds or removes an address from the whitelist.
+    /// @param target Address to add or remove from whitelist.
+    /// @param isApproved Whitelist status to assign to address.
     function updateWhitelistStatus(address target, bool isApproved)
         external
         onlyOwner
@@ -46,6 +54,14 @@ contract Whitelist is Ownable {
         isWhitelisted[target] = isApproved;
     }
 
+    /// @dev Fills an order using `msg.sender` as the taker.
+    ///      The transaction will revert if both the maker and taker are not whitelisted.
+    ///      Orders should specify this contract as the `senderAddress` in order to gaurantee
+    ///      that both maker and taker have been whitelisted.
+    /// @param order Order struct containing order specifications.
+    /// @param takerAssetFillAmount Desired amount of takerAsset to sell.
+    /// @param salt Arbitrary value to gaurantee uniqueness of 0x transaction hash.
+    /// @param orderSignature Proof that order has been created by maker.
     function fillOrderIfWhitelisted(
         LibOrder.Order memory order,
         uint256 takerAssetFillAmount,
@@ -58,23 +74,32 @@ contract Whitelist is Ownable {
         // This contract must be the entry point for the transaction.
         require(takerAddress == tx.origin);
 
-        // Check if sender is on the whitelist.
-        require(isWhitelisted[takerAddress]);
+        // Check if maker is on the whitelist
+        require(
+            isWhitelisted[order.makerAddress],
+            ADDRESS_NOT_WHITELISTED
+        );
+
+        // Check if taker is on the whitelist.
+        require(
+            isWhitelisted[takerAddress],
+            ADDRESS_NOT_WHITELISTED
+        );
 
         // Encode arguments into byte array.
         bytes memory data = abi.encodeWithSelector(
-            fillOrderFunctionSelector,
+            EXCHANGE.fillOrder.selector,
             order,
             takerAssetFillAmount,
             orderSignature
         );
 
-        // Call `fillOrder`via `executeTransaction`.
+        // Call `fillOrder` via `executeTransaction`.
         EXCHANGE.executeTransaction(
             salt,
             takerAddress,
             data,
-            txOriginSignature
+            TX_ORIGIN_SIGNATURE
         );
     }
 }
\ No newline at end of file
diff --git a/packages/contracts/src/utils/artifacts.ts b/packages/contracts/src/utils/artifacts.ts
index fe74ea072..357c66a0a 100644
--- a/packages/contracts/src/utils/artifacts.ts
+++ b/packages/contracts/src/utils/artifacts.ts
@@ -15,6 +15,7 @@ import * as TestLibs from '../artifacts/TestLibs.json';
 import * as TestSignatureValidator from '../artifacts/TestSignatureValidator.json';
 import * as TokenRegistry from '../artifacts/TokenRegistry.json';
 import * as EtherToken from '../artifacts/WETH9.json';
+import * as Whitelist from '../artifacts/Whitelist.json';
 import * as ZRX from '../artifacts/ZRXToken.json';
 
 export const artifacts = {
@@ -33,5 +34,6 @@ export const artifacts = {
     TestLibs: (TestLibs as any) as ContractArtifact,
     TestSignatureValidator: (TestSignatureValidator as any) as ContractArtifact,
     TokenRegistry: (TokenRegistry as any) as ContractArtifact,
+    Whitelist: (Whitelist as any) as ContractArtifact,
     ZRX: (ZRX as any) as ContractArtifact,
 };
diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts
index 7242b0063..fe6df2f75 100644
--- a/packages/contracts/test/exchange/transactions.ts
+++ b/packages/contracts/test/exchange/transactions.ts
@@ -212,8 +212,12 @@ describe('Exchange transactions', () => {
         let whitelistOrderFactory: OrderFactory;
 
         before(async () => {
-            const whitelistInstance = await deployer.deployAsync(ContractName.Whitelist, [exchange.address]);
-            whitelist = new WhitelistContract(whitelistInstance.abi, whitelistInstance.address, provider);
+            whitelist = await WhitelistContract.deployFrom0xArtifactAsync(
+                artifacts.Whitelist,
+                provider,
+                txDefaults,
+                exchange.address,
+            );
             const defaultOrderParams = {
                 ...constants.STATIC_ORDER_PARAMS,
                 senderAddress: whitelist.address,
@@ -231,7 +235,28 @@ describe('Exchange transactions', () => {
             erc20Balances = await erc20Wrapper.getBalancesAsync();
         });
 
+        it('should revert if maker has not been whitelisted', async () => {
+            const isApproved = true;
+            await whitelist.updateWhitelistStatus.sendTransactionAsync(takerAddress, isApproved, { from: owner });
+
+            const orderStruct = orderUtils.getOrderStruct(signedOrder);
+            const takerAssetFillAmount = signedOrder.takerAssetAmount;
+            const salt = ZeroEx.generatePseudoRandomSalt();
+            return expect(
+                whitelist.fillOrderIfWhitelisted.sendTransactionAsync(
+                    orderStruct,
+                    takerAssetFillAmount,
+                    salt,
+                    signedOrder.signature,
+                    { from: takerAddress },
+                ),
+            ).to.be.rejectedWith(constants.REVERT);
+        });
+
         it('should revert if taker has not been whitelisted', async () => {
+            const isApproved = true;
+            await whitelist.updateWhitelistStatus.sendTransactionAsync(makerAddress, isApproved, { from: owner });
+
             const orderStruct = orderUtils.getOrderStruct(signedOrder);
             const takerAssetFillAmount = signedOrder.takerAssetAmount;
             const salt = ZeroEx.generatePseudoRandomSalt();
@@ -246,8 +271,9 @@ describe('Exchange transactions', () => {
             ).to.be.rejectedWith(constants.REVERT);
         });
 
-        it('should fill the order if taker has been whitelisted', async () => {
+        it('should fill the order if maker and taker have been whitelisted', async () => {
             const isApproved = true;
+            await whitelist.updateWhitelistStatus.sendTransactionAsync(makerAddress, isApproved, { from: owner });
             await whitelist.updateWhitelistStatus.sendTransactionAsync(takerAddress, isApproved, { from: owner });
 
             const orderStruct = orderUtils.getOrderStruct(signedOrder);
@@ -260,7 +286,6 @@ describe('Exchange transactions', () => {
                 signedOrder.signature,
                 { from: takerAddress },
             );
-
             const newBalances = await erc20Wrapper.getBalancesAsync();
 
             const makerAssetFillAmount = signedOrder.makerAssetAmount;