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

4 files changed, 43 insertions, 12 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,
 };