diff options
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; |