From 5a840c88b5b767844cfcbf32b97b4123de1757fe Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 30 May 2018 10:00:58 -0700 Subject: Change logDecoder back into class, remove awaitTransactionMined from multiSigWrapper --- packages/contracts/src/utils/exchange_wrapper.ts | 42 +++++++++--------- packages/contracts/src/utils/log_decoder.ts | 52 ++++++++++++++++------- packages/contracts/src/utils/multi_sig_wrapper.ts | 18 +++----- 3 files changed, 62 insertions(+), 50 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/utils/exchange_wrapper.ts b/packages/contracts/src/utils/exchange_wrapper.ts index 0446f35d1..ca587f220 100644 --- a/packages/contracts/src/utils/exchange_wrapper.ts +++ b/packages/contracts/src/utils/exchange_wrapper.ts @@ -7,16 +7,18 @@ import { ExchangeContract } from '../contract_wrappers/generated/exchange'; import { constants } from './constants'; import { formatters } from './formatters'; -import { logDecoder } from './log_decoder'; +import { LogDecoder } from './log_decoder'; import { orderUtils } from './order_utils'; import { AssetProxyId, OrderInfo, SignedTransaction } from './types'; export class ExchangeWrapper { private _exchange: ExchangeContract; private _web3Wrapper: Web3Wrapper; + private _logDecoder: LogDecoder; constructor(exchangeContract: ExchangeContract, provider: Provider) { this._exchange = exchangeContract; this._web3Wrapper = new Web3Wrapper(provider); + this._logDecoder = new LogDecoder(this._web3Wrapper, this._exchange.address); } public async fillOrderAsync( signedOrder: SignedOrder, @@ -30,13 +32,13 @@ export class ExchangeWrapper { params.signature, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async cancelOrderAsync(signedOrder: SignedOrder, from: string): Promise { const params = orderUtils.createCancel(signedOrder); const txHash = await this._exchange.cancelOrder.sendTransactionAsync(params.order, { from }); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async fillOrKillOrderAsync( @@ -51,7 +53,7 @@ export class ExchangeWrapper { params.signature, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async fillOrderNoThrowAsync( @@ -66,7 +68,7 @@ export class ExchangeWrapper { params.signature, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async batchFillOrdersAsync( @@ -81,7 +83,7 @@ export class ExchangeWrapper { params.signatures, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async batchFillOrKillOrdersAsync( @@ -96,7 +98,7 @@ export class ExchangeWrapper { params.signatures, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async batchFillOrdersNoThrowAsync( @@ -111,7 +113,7 @@ export class ExchangeWrapper { params.signatures, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async marketSellOrdersAsync( @@ -126,7 +128,7 @@ export class ExchangeWrapper { params.signatures, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async marketSellOrdersNoThrowAsync( @@ -141,7 +143,7 @@ export class ExchangeWrapper { params.signatures, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async marketBuyOrdersAsync( @@ -156,7 +158,7 @@ export class ExchangeWrapper { params.signatures, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async marketBuyOrdersNoThrowAsync( @@ -171,7 +173,7 @@ export class ExchangeWrapper { params.signatures, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async batchCancelOrdersAsync( @@ -180,12 +182,12 @@ export class ExchangeWrapper { ): Promise { const params = formatters.createBatchCancel(orders); const txHash = await this._exchange.batchCancelOrders.sendTransactionAsync(params.orders, { from }); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async cancelOrdersUpToAsync(salt: BigNumber, from: string): Promise { const txHash = await this._exchange.cancelOrdersUpTo.sendTransactionAsync(salt, { from }); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async registerAssetProxyAsync( @@ -203,7 +205,7 @@ export class ExchangeWrapper { oldAssetProxyAddress, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async executeTransactionAsync( @@ -217,7 +219,7 @@ export class ExchangeWrapper { signedTx.signature, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async getTakerAssetFilledAmountAsync(orderHashHex: string): Promise { @@ -241,13 +243,7 @@ export class ExchangeWrapper { params.rightSignature, { from }, ); - const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); - return tx; - } - private async _getTxWithDecodedExchangeLogsAsync(txHash: string): Promise { - const tx = await this._web3Wrapper.awaitTransactionSuccessAsync(txHash, constants.AWAIT_TRANSACTION_MINED_MS); - tx.logs = _.filter(tx.logs, log => log.address === this._exchange.address); - tx.logs = _.map(tx.logs, log => logDecoder.decodeLogOrThrow(log)); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } } diff --git a/packages/contracts/src/utils/log_decoder.ts b/packages/contracts/src/utils/log_decoder.ts index d2e65d176..32819b657 100644 --- a/packages/contracts/src/utils/log_decoder.ts +++ b/packages/contracts/src/utils/log_decoder.ts @@ -1,19 +1,23 @@ import { ContractArtifact } from '@0xproject/sol-compiler'; -import { AbiDefinition, LogEntry, LogWithDecodedArgs, RawLog } from '@0xproject/types'; +import { + AbiDefinition, + LogEntry, + LogWithDecodedArgs, + RawLog, + TransactionReceiptWithDecodedLogs, +} from '@0xproject/types'; import { AbiDecoder, BigNumber } from '@0xproject/utils'; +import { Web3Wrapper } from '@0xproject/web3-wrapper'; import * as _ from 'lodash'; import { artifacts } from './artifacts'; +import { constants } from './constants'; -const abiArrays: AbiDefinition[][] = []; -_.forEach(artifacts, (artifact: ContractArtifact) => { - const compilerOutput = artifact.compilerOutput; - abiArrays.push(compilerOutput.abi); -}); -const abiDecoder = new AbiDecoder(abiArrays); - -export const logDecoder = { - wrapLogBigNumbers(log: any): any { +export class LogDecoder { + private _web3Wrapper: Web3Wrapper; + private _contractAddress: string; + private _abiDecoder: AbiDecoder; + public static wrapLogBigNumbers(log: any): any { const argNames = _.keys(log.args); for (const argName of argNames) { const isWeb3BigNumber = _.startsWith(log.args[argName].constructor.toString(), 'function BigNumber('); @@ -21,13 +25,29 @@ export const logDecoder = { log.args[argName] = new BigNumber(log.args[argName]); } } - }, - decodeLogOrThrow(log: LogEntry): LogWithDecodedArgs | RawLog { - const logWithDecodedArgsOrLog = abiDecoder.tryToDecodeLogOrNoop(log); + } + constructor(web3Wrapper: Web3Wrapper, contractAddress: string) { + this._web3Wrapper = web3Wrapper; + this._contractAddress = contractAddress; + const abiArrays: AbiDefinition[][] = []; + _.forEach(artifacts, (artifact: ContractArtifact) => { + const compilerOutput = artifact.compilerOutput; + abiArrays.push(compilerOutput.abi); + }); + this._abiDecoder = new AbiDecoder(abiArrays); + } + public decodeLogOrThrow(log: LogEntry): LogWithDecodedArgs | RawLog { + const logWithDecodedArgsOrLog = this._abiDecoder.tryToDecodeLogOrNoop(log); if (_.isUndefined((logWithDecodedArgsOrLog as LogWithDecodedArgs).args)) { throw new Error(`Unable to decode log: ${JSON.stringify(log)}`); } - logDecoder.wrapLogBigNumbers(logWithDecodedArgsOrLog); + LogDecoder.wrapLogBigNumbers(logWithDecodedArgsOrLog); return logWithDecodedArgsOrLog; - }, -}; + } + public async getTxWithDecodedLogsAsync(txHash: string): Promise { + const tx = await this._web3Wrapper.awaitTransactionSuccessAsync(txHash, constants.AWAIT_TRANSACTION_MINED_MS); + tx.logs = _.filter(tx.logs, log => log.address === this._contractAddress); + tx.logs = _.map(tx.logs, log => this.decodeLogOrThrow(log)); + return tx; + } +} diff --git a/packages/contracts/src/utils/multi_sig_wrapper.ts b/packages/contracts/src/utils/multi_sig_wrapper.ts index 5c73cdf5a..d67692194 100644 --- a/packages/contracts/src/utils/multi_sig_wrapper.ts +++ b/packages/contracts/src/utils/multi_sig_wrapper.ts @@ -7,14 +7,16 @@ import { AssetProxyOwnerContract } from '../contract_wrappers/generated/asset_pr import { MultiSigWalletContract } from '../contract_wrappers/generated/multi_sig_wallet'; import { constants } from './constants'; -import { logDecoder } from './log_decoder'; +import { LogDecoder } from './log_decoder'; export class MultiSigWrapper { private _multiSig: MultiSigWalletContract; private _web3Wrapper: Web3Wrapper; + private _logDecoder: LogDecoder; constructor(multiSigContract: MultiSigWalletContract, provider: Provider) { this._multiSig = multiSigContract; this._web3Wrapper = new Web3Wrapper(provider); + this._logDecoder = new LogDecoder(this._web3Wrapper, this._multiSig.address); } public async submitTransactionAsync( destination: string, @@ -26,17 +28,17 @@ export class MultiSigWrapper { const txHash = await this._multiSig.submitTransaction.sendTransactionAsync(destination, value, data, { from, }); - const tx = await this._getTxWithDecodedMultiSigLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async confirmTransactionAsync(txId: BigNumber, from: string): Promise { const txHash = await this._multiSig.confirmTransaction.sendTransactionAsync(txId, { from }); - const tx = await this._getTxWithDecodedMultiSigLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async executeTransactionAsync(txId: BigNumber, from: string): Promise { const txHash = await this._multiSig.executeTransaction.sendTransactionAsync(txId, { from }); - const tx = await this._getTxWithDecodedMultiSigLogsAsync(txHash); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } public async executeRemoveAuthorizedAddressAsync( @@ -45,13 +47,7 @@ export class MultiSigWrapper { ): Promise { const txHash = await (this ._multiSig as AssetProxyOwnerContract).executeRemoveAuthorizedAddress.sendTransactionAsync(txId, { from }); - const tx = await this._getTxWithDecodedMultiSigLogsAsync(txHash); - return tx; - } - private async _getTxWithDecodedMultiSigLogsAsync(txHash: string): Promise { - const tx = await this._web3Wrapper.awaitTransactionMinedAsync(txHash, constants.AWAIT_TRANSACTION_MINED_MS); - tx.logs = _.filter(tx.logs, log => log.address === this._multiSig.address); - tx.logs = _.map(tx.logs, log => logDecoder.decodeLogOrThrow(log)); + const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash); return tx; } } -- cgit v1.2.3 From a5a7217c8f24fe98275fd9927ca8c5e5f140c6f3 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 9 May 2018 16:27:50 -0700 Subject: Add deepCopyBytes method to LibBytes --- .../current/test/TestLibBytes/TestLibBytes.sol | 17 +++++++++ .../contracts/current/utils/LibBytes/LibBytes.sol | 41 ++++++++++++++++++++++ 2 files changed, 58 insertions(+) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol index 5a6801262..b3ed1f620 100644 --- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol +++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol @@ -25,6 +25,23 @@ contract TestLibBytes is LibBytes { + /// @dev Performs a deep copy of a section of a byte array. + /// @param b Byte array that will be sliced. + /// @param startIndex Index of first byte to copy. + /// @param endIndex Index of last byte to copy + 1. + /// @return A deep copy of b from startIndex to endIndex. + function publicDeepCopyBytes( + bytes memory b, + uint256 startIndex, + uint256 endIndex) + public + pure + returns (bytes memory copy) + { + copy = deepCopyBytes(b, startIndex, endIndex); + return copy; + } + /// @dev Tests equality of two byte arrays. /// @param lhs First byte array to compare. /// @param rhs Second byte array to compare. diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol index 975565773..527f7a7a9 100644 --- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol +++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol @@ -21,9 +21,50 @@ pragma solidity ^0.4.24; contract LibBytes { // Revert reasons + string constant GT_ZERO_LENGTH_REQUIRED = "Length must be greater than 0."; string constant GTE_4_LENGTH_REQUIRED = "Length must be greater than or equal to 4."; string constant GTE_20_LENGTH_REQUIRED = "Length must be greater than or equal to 20."; string constant GTE_32_LENGTH_REQUIRED = "Length must be greater than or equal to 32."; + string constant INDEX_OUT_OF_BOUNDS = "Specified array index is out of bounds."; + + /// @dev Performs a deep copy of a section of a byte array. + /// @param b Byte array that will be copied. + /// @param index Index of first byte to copy. + /// @param len Number of bytes to copy starting from index. + /// @return A deep copy `len` bytes of b starting from index. + function deepCopyBytes( + bytes memory b, + uint256 index, + uint256 len) + internal + pure + returns (bytes memory) + { + require( + len > 0, + GT_ZERO_LENGTH_REQUIRED + ); + require( + b.length >= index + len, + INDEX_OUT_OF_BOUNDS + ); + + bytes memory copy = new bytes(len); + + // Arrays are prefixed by a 256 bit length parameter + index += 32; + + assembly { + // Start storing onto copy after length field + let storeStartIndex := add(copy, 32) + // Start loading from b at index + let startLoadIndex := add(b, index) + for {let i := 0} lt(i, len) {i := add(i, 32)} { + mstore(add(storeStartIndex, i), mload(add(startLoadIndex, i))) + } + } + return copy; + } /// @dev Tests equality of two byte arrays. /// @param lhs First byte array to compare. -- cgit v1.2.3 From b587f076fe02a21274c91d35b6a18a7ba86e3b58 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 9 May 2018 16:42:25 -0700 Subject: Add Validator signature type --- .../protocol/Exchange/MixinSignatureValidator.sol | 34 +++++++++++++++++++-- .../protocol/Exchange/interfaces/IValidator.sol | 35 ++++++++++++++++++++++ .../Exchange/mixins/MSignatureValidator.sol | 1 + 3 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IValidator.sol (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index f7fcd36b6..2a54da90f 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -20,6 +20,7 @@ pragma solidity ^0.4.24; import "./mixins/MSignatureValidator.sol"; import "./interfaces/ISigner.sol"; +import "./interfaces/IValidator.sol"; import "./libs/LibExchangeErrors.sol"; import "../../utils/LibBytes/LibBytes.sol"; @@ -169,9 +170,38 @@ contract MixinSignatureValidator is isValid = signer == recovered; return isValid; - // Signature verified by signer contract + // Signature verified by signer contract. + // If used with an order, the maker of the order is the signer contract. } else if (signatureType == SignatureType.Contract) { - isValid = ISigner(signer).isValidSignature(hash, signature); + // Pass in signature without signature type. + bytes memory signatureWithoutType = deepCopyBytes( + signature, + 1, + signature.length - 1 + ); + isValid = ISigner(signer).isValidSignature(hash, signatureWithoutType); + return isValid; + + // Signature verified by validator contract. + // If used with an order, the maker of the order can still be an EOA. + // A signature using this type should be encoded as: + // | Offset | Length | Contents | + // | 0x00 | 1 | Signature type is always "\x07" | + // | 0x01 | 20 | Address of validator contract | + // | 0x15 | ** | Signature to validate | + } else if (signatureType == SignatureType.Validator) { + address validator = readAddress(signature, 1); + // Pass in signature without type or validator address. + bytes memory signatureWithoutTypeOrAddress = deepCopyBytes( + signature, + 21, + signature.length - 21 + ); + isValid = IValidator(validator).isValidSignature( + hash, + signer, + signatureWithoutTypeOrAddress + ); return isValid; // Signer signed hash previously using the preSign function diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IValidator.sol new file mode 100644 index 000000000..aed725066 --- /dev/null +++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IValidator.sol @@ -0,0 +1,35 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.4.23; + +contract IValidator { + + /// @dev Verifies that a signature is valid. + /// @param hash Message hash that is signed. + /// @param signer Address that should have signed the given hash. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidSignature( + bytes32 hash, + address signer, + bytes signature) + external + view + returns (bool isValid); +} diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol index 3658e7c6f..083fa89d7 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol @@ -32,6 +32,7 @@ contract MSignatureValidator is EIP712, Trezor, Contract, + Validator, PreSigned } -- cgit v1.2.3 From 0789c6a3d893eb1b1d9636a2d10e8d495b0aa13b Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Sun, 20 May 2018 20:33:41 -0700 Subject: Add approveValidator function --- .../protocol/Exchange/MixinSignatureValidator.sol | 17 ++++++++++++++++- .../protocol/Exchange/mixins/MSignatureValidator.sol | 2 +- 2 files changed, 17 insertions(+), 2 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 2a54da90f..76de961bc 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -33,6 +33,9 @@ contract MixinSignatureValidator is // Mapping of hash => signer => signed mapping(bytes32 => mapping(address => bool)) preSigned; + // Mapping of signer => validator => approved + mapping(address => mapping (address => bool)) allowedValidators; + /// @dev Approves a hash on-chain using any valid signature type. /// After presigning a hash, the preSign signature type will become valid for that hash and signer. /// @param signer Address that should have signed the given hash. @@ -50,6 +53,15 @@ contract MixinSignatureValidator is preSigned[hash][signer] = true; } + /// @dev Approves a Validator contract to verify signatures on signer's behalf. + /// @param validator Address of Validator contract. + /// @param approval Approval or disapproval of Validator contract. + function approveSignatureValidator(address validator, bool approval) + external + { + allowedValidators[msg.sender][validator] = approval; + } + /// @dev Verifies that a hash has been signed by the given signer. /// @param hash Any 32 byte hash. /// @param signer Address that should have signed the given hash. @@ -172,7 +184,7 @@ contract MixinSignatureValidator is // Signature verified by signer contract. // If used with an order, the maker of the order is the signer contract. - } else if (signatureType == SignatureType.Contract) { + } else if (signatureType == SignatureType.Signer) { // Pass in signature without signature type. bytes memory signatureWithoutType = deepCopyBytes( signature, @@ -191,6 +203,9 @@ contract MixinSignatureValidator is // | 0x15 | ** | Signature to validate | } else if (signatureType == SignatureType.Validator) { address validator = readAddress(signature, 1); + if (!allowedValidators[signer][validator]) { + return false; + } // Pass in signature without type or validator address. bytes memory signatureWithoutTypeOrAddress = deepCopyBytes( signature, diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol index 083fa89d7..33424ff84 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol @@ -31,7 +31,7 @@ contract MSignatureValidator is Ecrecover, EIP712, Trezor, - Contract, + Signer, Validator, PreSigned } -- cgit v1.2.3 From 3eb05b45053748f227bc984458580a33596800d1 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 26 Apr 2018 12:09:48 -0700 Subject: Add TxOrigin signature type and rearrange order of types --- .../protocol/Exchange/MixinSignatureValidator.sol | 113 +++++++++++---------- .../Exchange/mixins/MSignatureValidator.sol | 19 ++-- packages/contracts/src/utils/types.ts | 8 +- 3 files changed, 77 insertions(+), 63 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 76de961bc..1022b1fe7 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -83,13 +83,13 @@ contract MixinSignatureValidator is ); SignatureType signatureType = SignatureType(uint8(signature[0])); - // Variables are not scoped in Solidity + // Variables are not scoped in Solidity. uint8 v; bytes32 r; bytes32 s; address recovered; - // Always illegal signature + // Always illegal signature. // This is always an implicit option since a signer can create a // signature array with invalid type or length. We may as well make // it an explicit option. This aids testing and analysis. It is @@ -98,7 +98,7 @@ contract MixinSignatureValidator is // NOTE: Reason cannot be assigned to a variable because of https://github.com/ethereum/solidity/issues/4051 revert("Illegal signature type."); - // Always invalid signature + // Always invalid signature. // Like Illegal, this is always implicitly available and therefore // offered explicitly. It can be implicitly created by providing // a correctly formatted but incorrect signature. @@ -110,20 +110,14 @@ contract MixinSignatureValidator is isValid = false; return isValid; - // Implicitly signed by caller - // The signer has initiated the call. In the case of non-contract - // accounts it means the transaction itself was signed. - // Example: let's say for a particular operation three signatures - // A, B and C are required. To submit the transaction, A and B can - // give a signature to C, who can then submit the transaction using - // `Caller` for his own signature. Or A and C can sign and B can - // submit using `Caller`. Having `Caller` allows this flexibility. - } else if (signatureType == SignatureType.Caller) { - require( - signature.length == 1, - INVALID_SIGNATURE_LENGTH - ); - isValid = signer == msg.sender; + // Signature using EIP712 + } else if (signatureType == SignatureType.EIP712) { + require(signature.length == 66); + v = uint8(signature[1]); + r = get32(signature, 2); + s = get32(signature, 34); + recovered = ecrecover(hash, v, r, s); + isValid = signer == recovered; return isValid; // Signed using web3.eth_sign @@ -144,42 +138,29 @@ contract MixinSignatureValidator is isValid = signer == recovered; return isValid; - // Signature using EIP712 - } else if (signatureType == SignatureType.EIP712) { - require( - signature.length == 66, - INVALID_SIGNATURE_LENGTH - ); - v = uint8(signature[1]); - r = readBytes32(signature, 2); - s = readBytes32(signature, 34); - recovered = ecrecover(hash, v, r, s); - isValid = signer == recovered; + // Implicitly signed by signer of Ethereum transaction. + // This is useful when initiating a call from a contract that might + // otherwise require multiple signatures. + // Example: Contract A calls Exchange.fillOrder using `executeTransaction`. + // This would normally require the user to sign both an Ethereum transaction + // and a 0x transaction. By using the TxOrigin signature type, the signature + // for the Ethereum transaction will encompass both signatures. + } else if (signatureType == SignatureType.TxOrigin) { + require(signature.length == 1); + isValid = signer == tx.origin; return isValid; - // Signature from Trezor hardware wallet - // It differs from web3.eth_sign in the encoding of message length - // (Bitcoin varint encoding vs ascii-decimal, the latter is not - // self-terminating which leads to ambiguities). - // See also: - // https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer - // https://github.com/trezor/trezor-mcu/blob/master/firmware/ethereum.c#L602 - // https://github.com/trezor/trezor-mcu/blob/master/firmware/crypto.c#L36 - } else if (signatureType == SignatureType.Trezor) { - require( - signature.length == 66, - INVALID_SIGNATURE_LENGTH - ); - v = uint8(signature[1]); - r = readBytes32(signature, 2); - s = readBytes32(signature, 34); - recovered = ecrecover( - keccak256("\x19Ethereum Signed Message:\n\x41", hash), - v, - r, - s - ); - isValid = signer == recovered; + // Implicitly signed by caller. + // The signer has initiated the call. In the case of non-contract + // accounts it means the transaction itself was signed. + // Example: let's say for a particular operation three signatures + // A, B and C are required. To submit the transaction, A and B can + // give a signature to C, who can then submit the transaction using + // `Caller` for his own signature. Or A and C can sign and B can + // submit using `Caller`. Having `Caller` allows this flexibility. + } else if (signatureType == SignatureType.Caller) { + require(signature.length == 1); + isValid = signer == msg.sender; return isValid; // Signature verified by signer contract. @@ -219,6 +200,36 @@ contract MixinSignatureValidator is ); return isValid; + // Signer signed hash previously using the preSign function. + } else if (signatureType == SignatureType.PreSigned) { + isValid = preSigned[hash][signer]; + return isValid; + + // Signature from Trezor hardware wallet. + // It differs from web3.eth_sign in the encoding of message length + // (Bitcoin varint encoding vs ascii-decimal, the latter is not + // self-terminating which leads to ambiguities). + // See also: + // https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer + // https://github.com/trezor/trezor-mcu/blob/master/firmware/ethereum.c#L602 + // https://github.com/trezor/trezor-mcu/blob/master/firmware/crypto.c#L36 + } else if (signatureType == SignatureType.Trezor) { + require( + signature.length == 66, + INVALID_SIGNATURE_LENGTH + ); + v = uint8(signature[1]); + r = readBytes32(signature, 2); + s = readBytes32(signature, 34); + recovered = ecrecover( + keccak256("\x19Ethereum Signed Message:\n\x41", hash), + v, + r, + s + ); + isValid = signer == recovered; + return isValid; + // Signer signed hash previously using the preSign function } else if (signatureType == SignatureType.PreSigned) { isValid = preSigned[hash][signer]; diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol index 33424ff84..a13f4524b 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol @@ -25,15 +25,16 @@ contract MSignatureValidator is { // Allowed signature types. enum SignatureType { - Illegal, // Default value - Invalid, - Caller, - Ecrecover, - EIP712, - Trezor, - Signer, - Validator, - PreSigned + Illegal, // 0x00, default value + Invalid, // 0x01 + EIP712, // 0x02 + Ecrecover, // 0x03 + TxOrigin, // 0x04 + Caller, // 0x05 + Signer, // 0x06 + Validator, // 0x07 + PreSigned, // 0x08 + Trezor // 0x09 } /// @dev Verifies that a signature is valid. diff --git a/packages/contracts/src/utils/types.ts b/packages/contracts/src/utils/types.ts index 90f90ec27..cd2d128f1 100644 --- a/packages/contracts/src/utils/types.ts +++ b/packages/contracts/src/utils/types.ts @@ -116,11 +116,13 @@ export enum ContractName { export enum SignatureType { Illegal, Invalid, - Caller, - Ecrecover, EIP712, - Trezor, + Ecrecover, + TxOrigin, + Caller, Contract, + PreSigned, + Trezor, } export interface SignedTransaction { -- cgit v1.2.3 From 87d36f06fd7116afb62b65cdbc013d93a61d1969 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 26 Apr 2018 12:10:04 -0700 Subject: Add sample whitelist contract --- .../contracts/current/test/Whitelist/Whitelist.sol | 58 ++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol new file mode 100644 index 000000000..02158485e --- /dev/null +++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol @@ -0,0 +1,58 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.4.21; +pragma experimental ABIEncoderV2; + +import "../../protocol/Exchange/mixins/MTransactions.sol"; +import "../../protocol/Exchange/LibOrder.sol"; +import "../../utils/Ownable/Ownable.sol"; + +contract Whitelist is Ownable { + + mapping (address => bool) public isWhitelisted; + MTransactions EXCHANGE; + + bytes txOriginSignatureType = new bytes(1); + + function Whitelist(address _exchange) + public + { + EXCHANGE = MTransactions(_exchange); + txOriginSignatureType[0] = 0x04; + } + + function updateWhitelistStatus(address target, bool isApproved) + external + onlyOwner + { + isWhitelisted[target] = isApproved; + } + + function fillOrderIfWhitelisted( + LibOrder.Order memory order, + uint256 takerAssetFillAmount, + uint256 salt, + bytes memory signature) + public + { + require(isWhitelisted[msg.sender]); + bytes memory data = abi.encode(order, takerAssetFillAmount, signature); + EXCHANGE.executeTransaction(salt, msg.sender, data, txOriginSignatureType); + } +} \ No newline at end of file -- cgit v1.2.3 From d6be6f79ce944258193d01fa3a23c5210e17ed49 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 26 Apr 2018 12:55:16 -0700 Subject: Add example whitelist contract and minimum tests --- .../contracts/current/test/Whitelist/Whitelist.sol | 40 +++++++++++++++++----- packages/contracts/src/utils/types.ts | 1 + 2 files changed, 32 insertions(+), 9 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol index 02158485e..6955b6d96 100644 --- a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol +++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol @@ -19,22 +19,24 @@ pragma solidity ^0.4.21; pragma experimental ABIEncoderV2; -import "../../protocol/Exchange/mixins/MTransactions.sol"; +import "../../protocol/Exchange/Exchange.sol"; import "../../protocol/Exchange/LibOrder.sol"; import "../../utils/Ownable/Ownable.sol"; contract Whitelist is Ownable { mapping (address => bool) public isWhitelisted; - MTransactions EXCHANGE; + Exchange EXCHANGE; - bytes txOriginSignatureType = new bytes(1); + bytes txOriginSignature = new bytes(1); + bytes4 fillOrderFunctionSelector; function Whitelist(address _exchange) public { - EXCHANGE = MTransactions(_exchange); - txOriginSignatureType[0] = 0x04; + EXCHANGE = Exchange(_exchange); + txOriginSignature[0] = 0x04; + fillOrderFunctionSelector = EXCHANGE.fillOrder.selector; } function updateWhitelistStatus(address target, bool isApproved) @@ -48,11 +50,31 @@ contract Whitelist is Ownable { LibOrder.Order memory order, uint256 takerAssetFillAmount, uint256 salt, - bytes memory signature) + bytes memory orderSignature) public { - require(isWhitelisted[msg.sender]); - bytes memory data = abi.encode(order, takerAssetFillAmount, signature); - EXCHANGE.executeTransaction(salt, msg.sender, data, txOriginSignatureType); + address takerAddress = msg.sender; + + // This contract must be the entry point for the transaction. + require(takerAddress == tx.origin); + + // Check if sender is on the whitelist. + require(isWhitelisted[takerAddress]); + + // Encode arguments into byte array. + bytes memory data = abi.encodeWithSelector( + fillOrderFunctionSelector, + order, + takerAssetFillAmount, + orderSignature + ); + + // Call `fillOrder`via `executeTransaction`. + EXCHANGE.executeTransaction( + salt, + takerAddress, + data, + txOriginSignature + ); } } \ No newline at end of file diff --git a/packages/contracts/src/utils/types.ts b/packages/contracts/src/utils/types.ts index cd2d128f1..1eeffc70e 100644 --- a/packages/contracts/src/utils/types.ts +++ b/packages/contracts/src/utils/types.ts @@ -111,6 +111,7 @@ export enum ContractName { DummyERC721Token = 'DummyERC721Token', TestLibBytes = 'TestLibBytes', Authorizable = 'Authorizable', + Whitelist = 'Whitelist', } export enum SignatureType { -- cgit v1.2.3 From 34ab53173daad302e0d13cbf5369116b373d72d3 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 9 May 2018 13:51:26 -0700 Subject: Fix build --- .../protocol/Exchange/MixinSignatureValidator.sol | 19 ++++++++++++++----- .../Exchange/interfaces/IWrapperFunctions.sol | 14 +++++--------- .../contracts/current/test/Whitelist/Whitelist.sol | 10 +++++----- 3 files changed, 24 insertions(+), 19 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 1022b1fe7..1fb87c148 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -112,10 +112,13 @@ contract MixinSignatureValidator is // Signature using EIP712 } else if (signatureType == SignatureType.EIP712) { - require(signature.length == 66); + require( + signature.length == 66, + INVALID_SIGNATURE_LENGTH + ); v = uint8(signature[1]); - r = get32(signature, 2); - s = get32(signature, 34); + r = readBytes32(signature, 2); + s = readBytes32(signature, 34); recovered = ecrecover(hash, v, r, s); isValid = signer == recovered; return isValid; @@ -146,7 +149,10 @@ contract MixinSignatureValidator is // and a 0x transaction. By using the TxOrigin signature type, the signature // for the Ethereum transaction will encompass both signatures. } else if (signatureType == SignatureType.TxOrigin) { - require(signature.length == 1); + require( + signature.length == 1, + INVALID_SIGNATURE_LENGTH + ); isValid = signer == tx.origin; return isValid; @@ -159,7 +165,10 @@ contract MixinSignatureValidator is // `Caller` for his own signature. Or A and C can sign and B can // submit using `Caller`. Having `Caller` allows this flexibility. } else if (signatureType == SignatureType.Caller) { - require(signature.length == 1); + require( + signature.length == 1, + INVALID_SIGNATURE_LENGTH + ); isValid = signer == msg.sender; return isValid; diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IWrapperFunctions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IWrapperFunctions.sol index 1eb1233ed..8682b394a 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IWrapperFunctions.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IWrapperFunctions.sol @@ -19,27 +19,23 @@ pragma solidity ^0.4.24; pragma experimental ABIEncoderV2; -import "./libs/LibOrder.sol"; -import "./libs/LibFillResults.sol"; +import "../libs/LibOrder.sol"; +import "../libs/LibFillResults.sol"; contract IWrapperFunctions is - LibBytes, - LibMath, LibOrder, - LibFillResults, - LibExchangeErrors, - MExchangeCore + LibFillResults { /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled. /// @param order LibOrder.Order struct containing order specifications. /// @param takerAssetFillAmount Desired amount of takerAsset to sell. /// @param signature Proof that order has been created by maker. function fillOrKillOrder( - LibOrder.LibOrder.Order memory order, + LibOrder.Order memory order, uint256 takerAssetFillAmount, bytes memory signature) public - returns (LibFillResults.LibFillResults.FillResults memory fillResults); + returns (LibFillResults.FillResults memory fillResults); /// @dev Fills an order with specified parameters and ECDSA signature. /// Returns false if the transaction would otherwise revert. diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol index 6955b6d96..c53856602 100644 --- a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol +++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol @@ -16,17 +16,17 @@ */ -pragma solidity ^0.4.21; +pragma solidity ^0.4.23; pragma experimental ABIEncoderV2; -import "../../protocol/Exchange/Exchange.sol"; -import "../../protocol/Exchange/LibOrder.sol"; +import "../../protocol/Exchange/interfaces/IExchange.sol"; +import "../../protocol/Exchange/libs/LibOrder.sol"; import "../../utils/Ownable/Ownable.sol"; contract Whitelist is Ownable { mapping (address => bool) public isWhitelisted; - Exchange EXCHANGE; + IExchange EXCHANGE; bytes txOriginSignature = new bytes(1); bytes4 fillOrderFunctionSelector; @@ -34,7 +34,7 @@ contract Whitelist is Ownable { function Whitelist(address _exchange) public { - EXCHANGE = Exchange(_exchange); + EXCHANGE = IExchange(_exchange); txOriginSignature[0] = 0x04; fillOrderFunctionSelector = EXCHANGE.fillOrder.selector; } -- cgit v1.2.3 From 4b71c65aea44bba34429e288c5411a090dd78071 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 9 May 2018 14:41:44 -0700 Subject: Update Whitelist contract with comments, also require maker to be whitelisted --- .../protocol/Exchange/MixinTransactions.sol | 5 ++- .../protocol/Exchange/libs/LibExchangeErrors.sol | 1 + .../contracts/current/test/Whitelist/Whitelist.sol | 47 +++++++++++++++++----- packages/contracts/src/utils/artifacts.ts | 2 + 4 files changed, 43 insertions(+), 12 deletions(-) (limited to 'packages/contracts/src') 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, }; -- cgit v1.2.3 From 6d462fc961da2ba4d5502ad6b71654c1715550d9 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Mon, 21 May 2018 10:04:17 -0700 Subject: Remove TxOrigin signature type, modify whitelist to use Validator signature type --- .../protocol/Exchange/MixinSignatureValidator.sol | 21 +++------------------ .../Exchange/mixins/MSignatureValidator.sol | 11 +++++------ .../contracts/current/test/Whitelist/Whitelist.sol | 22 +++++++++++++++++++--- .../contracts/current/utils/LibBytes/LibBytes.sol | 4 ---- packages/contracts/src/utils/constants.ts | 1 + 5 files changed, 28 insertions(+), 31 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 1fb87c148..423f306cb 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -31,10 +31,10 @@ contract MixinSignatureValidator is { // Mapping of hash => signer => signed - mapping(bytes32 => mapping(address => bool)) preSigned; + mapping (bytes32 => mapping (address => bool)) preSigned; // Mapping of signer => validator => approved - mapping(address => mapping (address => bool)) allowedValidators; + mapping (address => mapping (address => bool)) allowedValidators; /// @dev Approves a hash on-chain using any valid signature type. /// After presigning a hash, the preSign signature type will become valid for that hash and signer. @@ -141,21 +141,6 @@ contract MixinSignatureValidator is isValid = signer == recovered; return isValid; - // Implicitly signed by signer of Ethereum transaction. - // This is useful when initiating a call from a contract that might - // otherwise require multiple signatures. - // Example: Contract A calls Exchange.fillOrder using `executeTransaction`. - // This would normally require the user to sign both an Ethereum transaction - // and a 0x transaction. By using the TxOrigin signature type, the signature - // for the Ethereum transaction will encompass both signatures. - } else if (signatureType == SignatureType.TxOrigin) { - require( - signature.length == 1, - INVALID_SIGNATURE_LENGTH - ); - isValid = signer == tx.origin; - return isValid; - // Implicitly signed by caller. // The signer has initiated the call. In the case of non-contract // accounts it means the transaction itself was signed. @@ -188,7 +173,7 @@ contract MixinSignatureValidator is // If used with an order, the maker of the order can still be an EOA. // A signature using this type should be encoded as: // | Offset | Length | Contents | - // | 0x00 | 1 | Signature type is always "\x07" | + // | 0x00 | 1 | Signature type is always "\x06" | // | 0x01 | 20 | Address of validator contract | // | 0x15 | ** | Signature to validate | } else if (signatureType == SignatureType.Validator) { diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol index a13f4524b..57600f508 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol @@ -29,12 +29,11 @@ contract MSignatureValidator is Invalid, // 0x01 EIP712, // 0x02 Ecrecover, // 0x03 - TxOrigin, // 0x04 - Caller, // 0x05 - Signer, // 0x06 - Validator, // 0x07 - PreSigned, // 0x08 - Trezor // 0x09 + Caller, // 0x04 + Signer, // 0x05 + Validator, // 0x06 + PreSigned, // 0x07 + Trezor // 0x08 } /// @dev Verifies that a signature is valid. diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol index 4a2e9a931..3bd3179e5 100644 --- a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol +++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol @@ -35,13 +35,14 @@ contract Whitelist is // Exchange contract. IExchange EXCHANGE; - // TxOrigin signature type is the 5th value in enum SignatureType and has a length of 1. - bytes constant TX_ORIGIN_SIGNATURE = "\x04"; + byte constant VALIDATOR_SIGNATURE_BYTE = "\x06"; + bytes TX_ORIGIN_SIGNATURE; constructor (address _exchange) public { EXCHANGE = IExchange(_exchange); + TX_ORIGIN_SIGNATURE = abi.encodePacked(VALIDATOR_SIGNATURE_BYTE, address(this)); } /// @dev Adds or removes an address from the whitelist. @@ -102,4 +103,19 @@ contract Whitelist is TX_ORIGIN_SIGNATURE ); } -} \ No newline at end of file + + /// @dev Verifies signer is same as signer of current Ethereum transaction. + /// @param signer Address that should have signed the given hash. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidSignature( + bytes32 hash, + address signer, + bytes signature) + external + view + returns (bool isValid) + { + return signer == tx.origin; + } +} diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol index 527f7a7a9..5c4fdbb23 100644 --- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol +++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol @@ -40,10 +40,6 @@ contract LibBytes { pure returns (bytes memory) { - require( - len > 0, - GT_ZERO_LENGTH_REQUIRED - ); require( b.length >= index + len, INDEX_OUT_OF_BOUNDS diff --git a/packages/contracts/src/utils/constants.ts b/packages/contracts/src/utils/constants.ts index 7a0e26a48..9b0b92545 100644 --- a/packages/contracts/src/utils/constants.ts +++ b/packages/contracts/src/utils/constants.ts @@ -28,6 +28,7 @@ export const constants = { DUMMY_TOKEN_SYMBOL: '', DUMMY_TOKEN_DECIMALS: new BigNumber(18), DUMMY_TOKEN_TOTAL_SUPPLY: new BigNumber(0), + NULL_BYTES: '0x', NUM_DUMMY_ERC20_TO_DEPLOY: 3, NUM_DUMMY_ERC721_TO_DEPLOY: 1, NUM_ERC721_TOKENS_TO_MINT: 2, -- cgit v1.2.3 From 822e319efea9c862702ddf589eb2344ff02e5bc5 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Mon, 21 May 2018 15:59:58 -0700 Subject: Use last byte of signature as signature type --- .../protocol/Exchange/MixinSignatureValidator.sol | 58 +++++++++------------- .../current/test/TestLibBytes/TestLibBytes.sol | 31 +++++++----- .../contracts/current/test/Whitelist/Whitelist.sol | 2 +- .../contracts/current/utils/LibBytes/LibBytes.sol | 54 +++++++++++--------- packages/contracts/src/utils/signing_utils.ts | 4 +- 5 files changed, 75 insertions(+), 74 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 423f306cb..6c018683e 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -81,7 +81,9 @@ contract MixinSignatureValidator is signature.length >= 1, INVALID_SIGNATURE_LENGTH ); - SignatureType signatureType = SignatureType(uint8(signature[0])); + + // Pop last byte off of + SignatureType signatureType = SignatureType(uint8(popByte(signature))); // Variables are not scoped in Solidity. uint8 v; @@ -104,7 +106,7 @@ contract MixinSignatureValidator is // a correctly formatted but incorrect signature. } else if (signatureType == SignatureType.Invalid) { require( - signature.length == 1, + signature.length == 0, INVALID_SIGNATURE_LENGTH ); isValid = false; @@ -113,12 +115,12 @@ contract MixinSignatureValidator is // Signature using EIP712 } else if (signatureType == SignatureType.EIP712) { require( - signature.length == 66, + signature.length == 65, INVALID_SIGNATURE_LENGTH ); - v = uint8(signature[1]); - r = readBytes32(signature, 2); - s = readBytes32(signature, 34); + v = uint8(signature[0]); + r = readBytes32(signature, 1); + s = readBytes32(signature, 33); recovered = ecrecover(hash, v, r, s); isValid = signer == recovered; return isValid; @@ -126,12 +128,12 @@ contract MixinSignatureValidator is // Signed using web3.eth_sign } else if (signatureType == SignatureType.Ecrecover) { require( - signature.length == 66, + signature.length == 65, INVALID_SIGNATURE_LENGTH ); - v = uint8(signature[1]); - r = readBytes32(signature, 2); - s = readBytes32(signature, 34); + v = uint8(signature[0]); + r = readBytes32(signature, 1); + s = readBytes32(signature, 33); recovered = ecrecover( keccak256("\x19Ethereum Signed Message:\n32", hash), v, @@ -151,7 +153,7 @@ contract MixinSignatureValidator is // submit using `Caller`. Having `Caller` allows this flexibility. } else if (signatureType == SignatureType.Caller) { require( - signature.length == 1, + signature.length == 0, INVALID_SIGNATURE_LENGTH ); isValid = signer == msg.sender; @@ -160,37 +162,25 @@ contract MixinSignatureValidator is // Signature verified by signer contract. // If used with an order, the maker of the order is the signer contract. } else if (signatureType == SignatureType.Signer) { - // Pass in signature without signature type. - bytes memory signatureWithoutType = deepCopyBytes( - signature, - 1, - signature.length - 1 - ); - isValid = ISigner(signer).isValidSignature(hash, signatureWithoutType); + isValid = ISigner(signer).isValidSignature(hash, signature); return isValid; // Signature verified by validator contract. // If used with an order, the maker of the order can still be an EOA. // A signature using this type should be encoded as: - // | Offset | Length | Contents | - // | 0x00 | 1 | Signature type is always "\x06" | - // | 0x01 | 20 | Address of validator contract | - // | 0x15 | ** | Signature to validate | + // | Offset | Length | Contents | + // | 0x00 | x | Signature to validate | + // | 0x00 + x | 20 | Address of validator contract | + // | 0x14 + x | 1 | Signature type is always "\x06" | } else if (signatureType == SignatureType.Validator) { - address validator = readAddress(signature, 1); + address validator = popAddress(signature); if (!allowedValidators[signer][validator]) { return false; } - // Pass in signature without type or validator address. - bytes memory signatureWithoutTypeOrAddress = deepCopyBytes( - signature, - 21, - signature.length - 21 - ); isValid = IValidator(validator).isValidSignature( hash, signer, - signatureWithoutTypeOrAddress + signature ); return isValid; @@ -209,12 +199,12 @@ contract MixinSignatureValidator is // https://github.com/trezor/trezor-mcu/blob/master/firmware/crypto.c#L36 } else if (signatureType == SignatureType.Trezor) { require( - signature.length == 66, + signature.length == 65, INVALID_SIGNATURE_LENGTH ); - v = uint8(signature[1]); - r = readBytes32(signature, 2); - s = readBytes32(signature, 34); + v = uint8(signature[0]); + r = readBytes32(signature, 1); + s = readBytes32(signature, 33); recovered = ecrecover( keccak256("\x19Ethereum Signed Message:\n\x41", hash), v, diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol index b3ed1f620..cd5206eb8 100644 --- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol +++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol @@ -25,21 +25,26 @@ contract TestLibBytes is LibBytes { - /// @dev Performs a deep copy of a section of a byte array. - /// @param b Byte array that will be sliced. - /// @param startIndex Index of first byte to copy. - /// @param endIndex Index of last byte to copy + 1. - /// @return A deep copy of b from startIndex to endIndex. - function publicDeepCopyBytes( - bytes memory b, - uint256 startIndex, - uint256 endIndex) + /// @dev Pops the last byte off of a byte array by modifying its length. + /// @param b Byte array that will be modified. + /// @return The byte that was popped off. + function publicPopByte(bytes memory b) public - pure - returns (bytes memory copy) + returns (bytes memory, byte result) + { + result = popByte(b); + return (b, result); + } + + /// @dev Pops the last 20 bytes off of a byte array by modifying its length. + /// @param b Byte array that will be modified. + /// @return The 20 byte address that was popped off. + function publicPopAddress(bytes memory b) + public + returns (bytes memory, address result) { - copy = deepCopyBytes(b, startIndex, endIndex); - return copy; + result = popAddress(b); + return (b, result); } /// @dev Tests equality of two byte arrays. diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol index 3bd3179e5..11576fd89 100644 --- a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol +++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol @@ -75,7 +75,7 @@ contract Whitelist is // This contract must be the entry point for the transaction. require(takerAddress == tx.origin); - // Check if maker is on the whitelist + // Check if maker is on the whitelist. require( isWhitelisted[order.makerAddress], ADDRESS_NOT_WHITELISTED diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol index 5c4fdbb23..f0a622fe4 100644 --- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol +++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol @@ -27,39 +27,45 @@ contract LibBytes { string constant GTE_32_LENGTH_REQUIRED = "Length must be greater than or equal to 32."; string constant INDEX_OUT_OF_BOUNDS = "Specified array index is out of bounds."; - /// @dev Performs a deep copy of a section of a byte array. - /// @param b Byte array that will be copied. - /// @param index Index of first byte to copy. - /// @param len Number of bytes to copy starting from index. - /// @return A deep copy `len` bytes of b starting from index. - function deepCopyBytes( - bytes memory b, - uint256 index, - uint256 len) + /// @dev Pops the last byte off of a byte array by modifying its length. + /// @param b Byte array that will be modified. + /// @return The byte that was popped off. + function popByte(bytes memory b) internal - pure - returns (bytes memory) + returns (byte result) { require( - b.length >= index + len, - INDEX_OUT_OF_BOUNDS + b.length > 0, + GT_ZERO_LENGTH_REQUIRED ); - bytes memory copy = new bytes(len); + result = b[b.length - 1]; + assembly { + let newLen := sub(mload(b), 1) + mstore(b, newLen) + } + result; + } - // Arrays are prefixed by a 256 bit length parameter - index += 32; + /// @dev Pops the last 20 bytes off of a byte array by modifying its length. + /// @param b Byte array that will be modified. + /// @return The 20 byte address that was popped off. + function popAddress(bytes memory b) + internal + returns (address result) + { + require( + b.length >= 20, + GTE_20_LENGTH_REQUIRED + ); + + result = readAddress(b, b.length - 20); assembly { - // Start storing onto copy after length field - let storeStartIndex := add(copy, 32) - // Start loading from b at index - let startLoadIndex := add(b, index) - for {let i := 0} lt(i, len) {i := add(i, 32)} { - mstore(add(storeStartIndex, i), mload(add(startLoadIndex, i))) - } + let newLen := sub(mload(b), 20) + mstore(b, newLen) } - return copy; + return result; } /// @dev Tests equality of two byte arrays. diff --git a/packages/contracts/src/utils/signing_utils.ts b/packages/contracts/src/utils/signing_utils.ts index 61ab1f138..4c36c8310 100644 --- a/packages/contracts/src/utils/signing_utils.ts +++ b/packages/contracts/src/utils/signing_utils.ts @@ -8,19 +8,19 @@ export const signingUtils = { const prefixedMessage = ethUtil.hashPersonalMessage(message); const ecSignature = ethUtil.ecsign(prefixedMessage, privateKey); const signature = Buffer.concat([ - ethUtil.toBuffer(signatureType), ethUtil.toBuffer(ecSignature.v), ecSignature.r, ecSignature.s, + ethUtil.toBuffer(signatureType), ]); return signature; } else if (signatureType === SignatureType.EIP712) { const ecSignature = ethUtil.ecsign(message, privateKey); const signature = Buffer.concat([ - ethUtil.toBuffer(signatureType), ethUtil.toBuffer(ecSignature.v), ecSignature.r, ecSignature.s, + ethUtil.toBuffer(signatureType), ]); return signature; } else { -- cgit v1.2.3 From fc5c598f8f6cc9c17407c8078101fa07356aa257 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 22 May 2018 09:58:08 -0700 Subject: Fix Exchange interface --- .../current/protocol/Exchange/interfaces/IExchange.sol | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IExchange.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IExchange.sol index fc428e9c0..9f21c18d7 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IExchange.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IExchange.sol @@ -20,17 +20,15 @@ pragma solidity ^0.4.24; pragma experimental ABIEncoderV2; import "./IExchangeCore.sol"; -import "./IMatchOrders"; -import "./ISettlement"; -import "./ISignatureValidator"; -import "./ITransactions"; -import "./IAssetProxyDispatcher"; -import "./IWrapperFunctions"; +import "./IMatchOrders.sol"; +import "./ISignatureValidator.sol"; +import "./ITransactions.sol"; +import "./IAssetProxyDispatcher.sol"; +import "./IWrapperFunctions.sol"; contract IExchange is IExchangeCore, IMatchOrders, - ISettlement, ISignatureValidator, ITransactions, IAssetProxyDispatcher, -- cgit v1.2.3 From ecdd0ce9f2378a1ac271b9eb5661a6d37d6edf41 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 22 May 2018 16:57:02 -0700 Subject: Update Whitelist --- .../protocol/Exchange/MixinSignatureValidator.sol | 18 ++++++++---- .../protocol/Exchange/MixinTransactions.sol | 3 +- .../current/test/TestLibBytes/TestLibBytes.sol | 2 +- .../contracts/current/test/Whitelist/Whitelist.sol | 21 ++++++++----- .../contracts/current/utils/LibBytes/LibBytes.sol | 34 ++++++++++++++++------ 5 files changed, 54 insertions(+), 24 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 6c018683e..8ef961a81 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -43,7 +43,8 @@ contract MixinSignatureValidator is function preSign( bytes32 hash, address signer, - bytes signature) + bytes signature + ) external { require( @@ -56,7 +57,10 @@ contract MixinSignatureValidator is /// @dev Approves a Validator contract to verify signatures on signer's behalf. /// @param validator Address of Validator contract. /// @param approval Approval or disapproval of Validator contract. - function approveSignatureValidator(address validator, bool approval) + function approveSignatureValidator( + address validator, + bool approval + ) external { allowedValidators[msg.sender][validator] = approval; @@ -70,19 +74,19 @@ contract MixinSignatureValidator is function isValidSignature( bytes32 hash, address signer, - bytes memory signature) + bytes memory signature + ) internal view returns (bool isValid) { // TODO: Domain separation: make hash depend on role. (Taker sig should not be valid as maker sig, etc.) - require( signature.length >= 1, INVALID_SIGNATURE_LENGTH ); - // Pop last byte off of + // Pop last byte off of signature byte array. SignatureType signatureType = SignatureType(uint8(popByte(signature))); // Variables are not scoped in Solidity. @@ -90,7 +94,7 @@ contract MixinSignatureValidator is bytes32 r; bytes32 s; address recovered; - + // Always illegal signature. // This is always an implicit option since a signer can create a // signature array with invalid type or length. We may as well make @@ -173,7 +177,9 @@ contract MixinSignatureValidator is // | 0x00 + x | 20 | Address of validator contract | // | 0x14 + x | 1 | Signature type is always "\x06" | } else if (signatureType == SignatureType.Validator) { + // Pop last 20 bytes off of signature byte array. address validator = popAddress(signature); + // Ensure signer has approved validator. if (!allowedValidators[signer][validator]) { return false; } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol index d28df11d8..12bba1ef2 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol @@ -43,7 +43,8 @@ contract MixinTransactions is uint256 salt, address signer, bytes data, - bytes signature) + bytes signature + ) external { // Prevent reentrancy diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol index cd5206eb8..b835e70d0 100644 --- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol +++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol @@ -30,7 +30,7 @@ contract TestLibBytes is /// @return The byte that was popped off. function publicPopByte(bytes memory b) public - returns (bytes memory, byte result) + returns (bytes memory, bytes1 result) { result = popByte(b); return (b, result); diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol index 11576fd89..034222915 100644 --- a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol +++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol @@ -27,7 +27,9 @@ contract Whitelist is Ownable { // Revert reasons - string constant ADDRESS_NOT_WHITELISTED = "Address not whitelisted."; + string constant MAKER_NOT_WHITELISTED = "Maker address not whitelisted."; + string constant TAKER_NOT_WHITELISTED = "Taker address not whitelisted."; + string constant INVALID_SENDER = "Sender must equal transaction origin."; // Mapping of address => whitelist status. mapping (address => bool) public isWhitelisted; @@ -42,7 +44,7 @@ contract Whitelist is public { EXCHANGE = IExchange(_exchange); - TX_ORIGIN_SIGNATURE = abi.encodePacked(VALIDATOR_SIGNATURE_BYTE, address(this)); + TX_ORIGIN_SIGNATURE = abi.encodePacked(address(this), VALIDATOR_SIGNATURE_BYTE); } /// @dev Adds or removes an address from the whitelist. @@ -67,24 +69,28 @@ contract Whitelist is LibOrder.Order memory order, uint256 takerAssetFillAmount, uint256 salt, - bytes memory orderSignature) + bytes memory orderSignature + ) public { address takerAddress = msg.sender; // This contract must be the entry point for the transaction. - require(takerAddress == tx.origin); + require( + takerAddress == tx.origin, + INVALID_SENDER + ); // Check if maker is on the whitelist. require( isWhitelisted[order.makerAddress], - ADDRESS_NOT_WHITELISTED + MAKER_NOT_WHITELISTED ); // Check if taker is on the whitelist. require( isWhitelisted[takerAddress], - ADDRESS_NOT_WHITELISTED + TAKER_NOT_WHITELISTED ); // Encode arguments into byte array. @@ -111,7 +117,8 @@ contract Whitelist is function isValidSignature( bytes32 hash, address signer, - bytes signature) + bytes signature + ) external view returns (bool isValid) diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol index f0a622fe4..df2221c93 100644 --- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol +++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol @@ -32,19 +32,23 @@ contract LibBytes { /// @return The byte that was popped off. function popByte(bytes memory b) internal - returns (byte result) + pure + returns (bytes1 result) { require( b.length > 0, GT_ZERO_LENGTH_REQUIRED ); + // Store last byte. result = b[b.length - 1]; + assembly { + // Decrement length of byte array. let newLen := sub(mload(b), 1) mstore(b, newLen) } - result; + return result; } /// @dev Pops the last 20 bytes off of a byte array by modifying its length. @@ -52,6 +56,7 @@ contract LibBytes { /// @return The 20 byte address that was popped off. function popAddress(bytes memory b) internal + pure returns (address result) { require( @@ -59,9 +64,11 @@ contract LibBytes { GTE_20_LENGTH_REQUIRED ); + // Store last 20 bytes. result = readAddress(b, b.length - 20); assembly { + // Subtract 20 from byte array length. let newLen := sub(mload(b), 20) mstore(b, newLen) } @@ -72,7 +79,10 @@ contract LibBytes { /// @param lhs First byte array to compare. /// @param rhs Second byte array to compare. /// @return True if arrays are the same. False otherwise. - function areBytesEqual(bytes memory lhs, bytes memory rhs) + function areBytesEqual( + bytes memory lhs, + bytes memory rhs + ) internal pure returns (bool equal) @@ -106,7 +116,8 @@ contract LibBytes { /// @return address from byte array. function readAddress( bytes memory b, - uint256 index) + uint256 index + ) internal pure returns (address result) @@ -138,7 +149,8 @@ contract LibBytes { function writeAddress( bytes memory b, uint256 index, - address input) + address input + ) internal pure { @@ -175,7 +187,8 @@ contract LibBytes { /// @return bytes32 value from byte array. function readBytes32( bytes memory b, - uint256 index) + uint256 index + ) internal pure returns (bytes32 result) @@ -202,7 +215,8 @@ contract LibBytes { function writeBytes32( bytes memory b, uint256 index, - bytes32 input) + bytes32 input + ) internal pure { @@ -226,7 +240,8 @@ contract LibBytes { /// @return uint256 value from byte array. function readUint256( bytes memory b, - uint256 index) + uint256 index + ) internal pure returns (uint256 result) @@ -241,7 +256,8 @@ contract LibBytes { function writeUint256( bytes memory b, uint256 index, - uint256 input) + uint256 input + ) internal pure { -- cgit v1.2.3 From e5b7e29113fb7c87f41e492abb1fd81247f0db46 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 23 May 2018 10:08:06 -0700 Subject: Add signer to txHash, allow approveValidator to be used with executeTransaction --- .../current/protocol/Exchange/MixinSignatureValidator.sol | 7 +++++-- .../contracts/current/protocol/Exchange/MixinTransactions.sol | 1 + packages/contracts/src/utils/transaction_factory.ts | 9 ++++----- 3 files changed, 10 insertions(+), 7 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 8ef961a81..e2d4808c3 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -19,6 +19,7 @@ pragma solidity ^0.4.24; import "./mixins/MSignatureValidator.sol"; +import "./mixins/MTransactions.sol"; import "./interfaces/ISigner.sol"; import "./interfaces/IValidator.sol"; import "./libs/LibExchangeErrors.sol"; @@ -27,7 +28,8 @@ import "../../utils/LibBytes/LibBytes.sol"; contract MixinSignatureValidator is LibBytes, LibExchangeErrors, - MSignatureValidator + MSignatureValidator, + MTransactions { // Mapping of hash => signer => signed @@ -63,7 +65,8 @@ contract MixinSignatureValidator is ) external { - allowedValidators[msg.sender][validator] = approval; + address signer = getCurrentContextAddress(); + allowedValidators[signer][validator] = approval; } /// @dev Verifies that a hash has been signed by the given signer. diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol index 12bba1ef2..d153dfa5c 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol @@ -56,6 +56,7 @@ contract MixinTransactions is // Calculate transaction hash bytes32 transactionHash = keccak256( address(this), + signer, salt, data ); diff --git a/packages/contracts/src/utils/transaction_factory.ts b/packages/contracts/src/utils/transaction_factory.ts index 941bff96d..65cdb3f89 100644 --- a/packages/contracts/src/utils/transaction_factory.ts +++ b/packages/contracts/src/utils/transaction_factory.ts @@ -7,26 +7,25 @@ import { signingUtils } from './signing_utils'; import { SignatureType, SignedTransaction } from './types'; export class TransactionFactory { - private _signer: string; + private _signerBuff: Buffer; private _exchangeAddress: string; private _privateKey: Buffer; constructor(privateKey: Buffer, exchangeAddress: string) { this._privateKey = privateKey; this._exchangeAddress = exchangeAddress; - const signerBuff = ethUtil.privateToAddress(this._privateKey); - this._signer = `0x${signerBuff.toString('hex')}`; + this._signerBuff = ethUtil.privateToAddress(this._privateKey); } public newSignedTransaction( data: string, signatureType: SignatureType = SignatureType.Ecrecover, ): SignedTransaction { const salt = generatePseudoRandomSalt(); - const txHash = crypto.solSHA3([this._exchangeAddress, salt, ethUtil.toBuffer(data)]); + const txHash = crypto.solSHA3([this._exchangeAddress, this._signerBuff, salt, ethUtil.toBuffer(data)]); const signature = signingUtils.signMessage(txHash, this._privateKey, signatureType); const signedTx = { exchangeAddress: this._exchangeAddress, salt, - signer: this._signer, + signer: `0x${this._signerBuff.toString('hex')}`, data, signature: `0x${signature.toString('hex')}`, }; -- cgit v1.2.3 From 6050a59e4a190cf8f8d42e390b435a7184b8f718 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 23 May 2018 15:20:47 -0700 Subject: Make AssetProxyId last byte of assetData --- .../contracts/current/protocol/AssetProxy/ERC20Proxy.sol | 10 ++++++---- .../current/protocol/AssetProxy/ERC721Proxy.sol | 12 +++++++----- .../current/protocol/AssetProxy/MixinAssetProxy.sol | 6 ++++-- .../current/protocol/AssetProxy/MixinAuthorizable.sol | 5 ++++- .../protocol/AssetProxy/interfaces/IAssetProxy.sol | 6 ++++-- .../protocol/AssetProxy/interfaces/IAuthorizable.sol | 5 ++++- .../current/protocol/AssetProxy/mixins/MAssetProxy.sol | 3 ++- .../current/protocol/AssetProxy/mixins/MAuthorizable.sol | 2 +- .../protocol/Exchange/MixinAssetProxyDispatcher.sol | 12 ++++++++---- .../protocol/Exchange/MixinSignatureValidator.sol | 2 +- .../protocol/Exchange/interfaces/ISignatureValidator.sol | 12 +++++++++++- .../current/protocol/Exchange/interfaces/ISigner.sol | 3 ++- .../current/protocol/Exchange/interfaces/IValidator.sol | 3 ++- .../protocol/Exchange/mixins/MAssetProxyDispatcher.sol | 3 ++- .../protocol/Exchange/mixins/MSignatureValidator.sol | 3 ++- .../TestSignatureValidator/TestSignatureValidator.sol | 3 ++- .../src/contracts/current/test/Whitelist/Whitelist.sol | 5 ++++- packages/contracts/src/utils/asset_proxy_utils.ts | 16 ++++++++-------- 18 files changed, 74 insertions(+), 37 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol index ee0c66fdc..7c25c9770 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol @@ -47,21 +47,23 @@ contract ERC20Proxy is bytes memory assetMetadata, address from, address to, - uint256 amount) + uint256 amount + ) internal { // Data must be intended for this proxy. + uint256 length = assetMetadata.length; require( - uint8(assetMetadata[0]) == PROXY_ID, + uint8(assetMetadata[length - 1]) == PROXY_ID, PROXY_ID_MISMATCH ); // Decode metadata. require( - assetMetadata.length == 21, + length == 21, INVALID_METADATA_LENGTH ); - address token = readAddress(assetMetadata, 1); + address token = readAddress(assetMetadata, 0); // Transfer tokens. bool success = IERC20Token(token).transferFrom(from, to, amount); diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol index 94aab9139..59045f73a 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol @@ -47,12 +47,14 @@ contract ERC721Proxy is bytes memory assetMetadata, address from, address to, - uint256 amount) + uint256 amount + ) internal { // Data must be intended for this proxy. + uint256 length = assetMetadata.length; require( - uint8(assetMetadata[0]) == PROXY_ID, + uint8(assetMetadata[length - 1]) == PROXY_ID, PROXY_ID_MISMATCH ); @@ -64,11 +66,11 @@ contract ERC721Proxy is // Decode metadata require( - assetMetadata.length == 53, + length == 53, INVALID_METADATA_LENGTH ); - address token = readAddress(assetMetadata, 1); - uint256 tokenId = readUint256(assetMetadata, 21); + address token = readAddress(assetMetadata, 0); + uint256 tokenId = readUint256(assetMetadata, 20); // Transfer token. // Either succeeds or throws. diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAssetProxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAssetProxy.sol index 4ec31304f..4bd533148 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAssetProxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAssetProxy.sol @@ -36,7 +36,8 @@ contract MixinAssetProxy is bytes assetMetadata, address from, address to, - uint256 amount) + uint256 amount + ) external onlyAuthorized { @@ -57,7 +58,8 @@ contract MixinAssetProxy is bytes[] memory assetMetadata, address[] memory from, address[] memory to, - uint256[] memory amounts) + uint256[] memory amounts + ) public onlyAuthorized { diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol index 0bbd3b318..4aaeb66d1 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol @@ -87,7 +87,10 @@ contract MixinAuthorizable is /// @dev Removes authorizion of an address. /// @param target Address to remove authorization from. /// @param index Index of target in authorities array. - function removeAuthorizedAddressAtIndex(address target, uint256 index) + function removeAuthorizedAddressAtIndex( + address target, + uint256 index + ) external { require( diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetProxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetProxy.sol index 8b30dfabb..7e1848889 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetProxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAssetProxy.sol @@ -34,7 +34,8 @@ contract IAssetProxy is bytes assetMetadata, address from, address to, - uint256 amount) + uint256 amount + ) external; /// @dev Makes multiple transfers of assets. Either succeeds or throws. @@ -46,7 +47,8 @@ contract IAssetProxy is bytes[] memory assetMetadata, address[] memory from, address[] memory to, - uint256[] memory amounts) + uint256[] memory amounts + ) public; /// @dev Gets the proxy id associated with the proxy address. diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAuthorizable.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAuthorizable.sol index d6fe03898..cedd1744c 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAuthorizable.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/interfaces/IAuthorizable.sol @@ -45,6 +45,9 @@ contract IAuthorizable is /// @dev Removes authorizion of an address. /// @param target Address to remove authorization from. /// @param index Index of target in authorities array. - function removeAuthorizedAddressAtIndex(address target, uint256 index) + function removeAuthorizedAddressAtIndex( + address target, + uint256 index + ) external; } diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/mixins/MAssetProxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/mixins/MAssetProxy.sol index 3800bf04c..de9d65a53 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/mixins/MAssetProxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/mixins/MAssetProxy.sol @@ -34,6 +34,7 @@ contract MAssetProxy is bytes memory assetMetadata, address from, address to, - uint256 amount) + uint256 amount + ) internal; } diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/mixins/MAuthorizable.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/mixins/MAuthorizable.sol index cdf60bdee..6f35bd7ec 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/mixins/MAuthorizable.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/mixins/MAuthorizable.sol @@ -38,5 +38,5 @@ contract MAuthorizable is ); /// @dev Only authorized addresses can invoke functions with this modifier. - modifier onlyAuthorized { _; } + modifier onlyAuthorized { revert(); _; } } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol index 3b38d1f37..d996f933c 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinAssetProxyDispatcher.sol @@ -39,7 +39,8 @@ contract MixinAssetProxyDispatcher is function registerAssetProxy( uint8 assetProxyId, address newAssetProxy, - address oldAssetProxy) + address oldAssetProxy + ) external onlyOwner { @@ -86,17 +87,20 @@ contract MixinAssetProxyDispatcher is bytes memory assetMetadata, address from, address to, - uint256 amount) + uint256 amount + ) internal { // Do nothing if no amount should be transferred. if (amount > 0) { + // Lookup asset proxy + uint256 length = assetMetadata.length; require( - assetMetadata.length >= 1, + length > 0, GT_ZERO_LENGTH_REQUIRED ); - uint8 assetProxyId = uint8(assetMetadata[0]); + uint8 assetProxyId = uint8(assetMetadata[length - 1]); IAssetProxy assetProxy = assetProxies[assetProxyId]; // transferFrom will either succeed or throw. diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index e2d4808c3..96a1f578a 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -105,7 +105,7 @@ contract MixinSignatureValidator is // also the initialization value for the enum type. if (signatureType == SignatureType.Illegal) { // NOTE: Reason cannot be assigned to a variable because of https://github.com/ethereum/solidity/issues/4051 - revert("Illegal signature type."); + revert(ILLEGAL_SIGNATURE_TYPE); // Always invalid signature. // Like Illegal, this is always implicitly available and therefore diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol index 65ff45f7b..72f15a2a4 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol @@ -27,6 +27,16 @@ contract ISignatureValidator { function preSign( bytes32 hash, address signer, - bytes signature) + bytes signature + ) + external; + + /// @dev Approves a Validator contract to verify signatures on signer's behalf. + /// @param validator Address of Validator contract. + /// @param approval Approval or disapproval of Validator contract. + function approveSignatureValidator( + address validator, + bool approval + ) external; } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISigner.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISigner.sol index 53c41d331..0186ad490 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISigner.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISigner.sol @@ -26,7 +26,8 @@ contract ISigner { /// @return Validity of order signature. function isValidSignature( bytes32 hash, - bytes signature) + bytes signature + ) external view returns (bool isValid); diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IValidator.sol index aed725066..3e5ccc190 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IValidator.sol @@ -28,7 +28,8 @@ contract IValidator { function isValidSignature( bytes32 hash, address signer, - bytes signature) + bytes signature + ) external view returns (bool isValid); diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol index ccc960d6e..87c5f6361 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MAssetProxyDispatcher.sol @@ -41,6 +41,7 @@ contract MAssetProxyDispatcher is bytes memory assetMetadata, address from, address to, - uint256 amount) + uint256 amount + ) internal; } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol index 57600f508..dc08c6d27 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol @@ -44,7 +44,8 @@ contract MSignatureValidator is function isValidSignature( bytes32 hash, address signer, - bytes memory signature) + bytes memory signature + ) internal view returns (bool isValid); diff --git a/packages/contracts/src/contracts/current/test/TestSignatureValidator/TestSignatureValidator.sol b/packages/contracts/src/contracts/current/test/TestSignatureValidator/TestSignatureValidator.sol index 15d9ca189..e2bc75b37 100644 --- a/packages/contracts/src/contracts/current/test/TestSignatureValidator/TestSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/test/TestSignatureValidator/TestSignatureValidator.sol @@ -26,7 +26,8 @@ contract TestSignatureValidator is MixinSignatureValidator { function publicIsValidSignature( bytes32 hash, address signer, - bytes memory signature) + bytes memory signature + ) public view returns (bool isValid) diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol index 034222915..8b9763c65 100644 --- a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol +++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol @@ -50,7 +50,10 @@ contract Whitelist is /// @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) + function updateWhitelistStatus( + address target, + bool isApproved + ) external onlyOwner { diff --git a/packages/contracts/src/utils/asset_proxy_utils.ts b/packages/contracts/src/utils/asset_proxy_utils.ts index c042da5d0..a17d4cdfa 100644 --- a/packages/contracts/src/utils/asset_proxy_utils.ts +++ b/packages/contracts/src/utils/asset_proxy_utils.ts @@ -39,7 +39,7 @@ export const assetProxyUtils = { encodeERC20ProxyData(tokenAddress: string): string { const encodedAssetProxyId = assetProxyUtils.encodeAssetProxyId(AssetProxyId.ERC20); const encodedAddress = assetProxyUtils.encodeAddress(tokenAddress); - const encodedMetadata = Buffer.concat([encodedAssetProxyId, encodedAddress]); + const encodedMetadata = Buffer.concat([encodedAddress, encodedAssetProxyId]); const encodedMetadataHex = ethUtil.bufferToHex(encodedMetadata); return encodedMetadataHex; }, @@ -52,7 +52,7 @@ export const assetProxyUtils = { }`, ); } - const encodedAssetProxyId = encodedProxyMetadata.slice(0, 1); + const encodedAssetProxyId = encodedProxyMetadata.slice(-1); const assetProxyId = assetProxyUtils.decodeAssetProxyId(encodedAssetProxyId); if (assetProxyId !== AssetProxyId.ERC20) { throw new Error( @@ -61,7 +61,7 @@ export const assetProxyUtils = { }), but got ${assetProxyId}`, ); } - const encodedTokenAddress = encodedProxyMetadata.slice(1, 21); + const encodedTokenAddress = encodedProxyMetadata.slice(0, 20); const tokenAddress = assetProxyUtils.decodeAddress(encodedTokenAddress); const erc20ProxyData = { assetProxyId, @@ -73,7 +73,7 @@ export const assetProxyUtils = { const encodedAssetProxyId = assetProxyUtils.encodeAssetProxyId(AssetProxyId.ERC721); const encodedAddress = assetProxyUtils.encodeAddress(tokenAddress); const encodedTokenId = assetProxyUtils.encodeUint256(tokenId); - const encodedMetadata = Buffer.concat([encodedAssetProxyId, encodedAddress, encodedTokenId]); + const encodedMetadata = Buffer.concat([encodedAddress, encodedTokenId, encodedAssetProxyId]); const encodedMetadataHex = ethUtil.bufferToHex(encodedMetadata); return encodedMetadataHex; }, @@ -86,7 +86,7 @@ export const assetProxyUtils = { }`, ); } - const encodedAssetProxyId = encodedProxyMetadata.slice(0, 1); + const encodedAssetProxyId = encodedProxyMetadata.slice(-1); const assetProxyId = assetProxyUtils.decodeAssetProxyId(encodedAssetProxyId); if (assetProxyId !== AssetProxyId.ERC721) { throw new Error( @@ -95,9 +95,9 @@ export const assetProxyUtils = { }), but got ${assetProxyId}`, ); } - const encodedTokenAddress = encodedProxyMetadata.slice(1, 21); + const encodedTokenAddress = encodedProxyMetadata.slice(0, 20); const tokenAddress = assetProxyUtils.decodeAddress(encodedTokenAddress); - const encodedTokenId = encodedProxyMetadata.slice(21, 53); + const encodedTokenId = encodedProxyMetadata.slice(20, 52); const tokenId = assetProxyUtils.decodeUint256(encodedTokenId); const erc721ProxyData = { assetProxyId, @@ -115,7 +115,7 @@ export const assetProxyUtils = { }`, ); } - const encodedAssetProxyId = encodedProxyMetadata.slice(0, 1); + const encodedAssetProxyId = encodedProxyMetadata.slice(-1); const assetProxyId = assetProxyUtils.decodeAssetProxyId(encodedAssetProxyId); return assetProxyId; }, -- cgit v1.2.3 From 9f93d8f53380bf650702c653fea13b3e7ed5f8f7 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 24 May 2018 10:24:32 -0700 Subject: Fix formatting and tests --- .../contracts/current/protocol/Exchange/MixinSignatureValidator.sol | 4 +--- .../src/contracts/current/test/TestLibBytes/TestLibBytes.sol | 2 ++ .../current/test/TestSignatureValidator/TestSignatureValidator.sol | 6 +++++- .../contracts/src/contracts/current/test/Whitelist/Whitelist.sol | 2 ++ 4 files changed, 10 insertions(+), 4 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 96a1f578a..4b0b1d02d 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -104,7 +104,6 @@ contract MixinSignatureValidator is // it an explicit option. This aids testing and analysis. It is // also the initialization value for the enum type. if (signatureType == SignatureType.Illegal) { - // NOTE: Reason cannot be assigned to a variable because of https://github.com/ethereum/solidity/issues/4051 revert(ILLEGAL_SIGNATURE_TYPE); // Always invalid signature. @@ -234,7 +233,6 @@ contract MixinSignatureValidator is // that we currently support. In this case returning false // may lead the caller to incorrectly believe that the // signature was invalid.) - // NOTE: Reason cannot be assigned to a variable because of https://github.com/ethereum/solidity/issues/4051 - revert("Unsupported signature type."); + revert(UNSUPPORTED_SIGNATURE_TYPE); } } diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol index b835e70d0..69554605d 100644 --- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol +++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol @@ -30,6 +30,7 @@ contract TestLibBytes is /// @return The byte that was popped off. function publicPopByte(bytes memory b) public + pure returns (bytes memory, bytes1 result) { result = popByte(b); @@ -41,6 +42,7 @@ contract TestLibBytes is /// @return The 20 byte address that was popped off. function publicPopAddress(bytes memory b) public + pure returns (bytes memory, address result) { result = popAddress(b); diff --git a/packages/contracts/src/contracts/current/test/TestSignatureValidator/TestSignatureValidator.sol b/packages/contracts/src/contracts/current/test/TestSignatureValidator/TestSignatureValidator.sol index e2bc75b37..0f84678cf 100644 --- a/packages/contracts/src/contracts/current/test/TestSignatureValidator/TestSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/test/TestSignatureValidator/TestSignatureValidator.sol @@ -20,8 +20,12 @@ pragma solidity ^0.4.24; pragma experimental ABIEncoderV2; import "../../protocol/Exchange/MixinSignatureValidator.sol"; +import "../../protocol/Exchange/MixinTransactions.sol"; -contract TestSignatureValidator is MixinSignatureValidator { +contract TestSignatureValidator is + MixinSignatureValidator, + MixinTransactions +{ function publicIsValidSignature( bytes32 hash, diff --git a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol index 8b9763c65..0594e2767 100644 --- a/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol +++ b/packages/contracts/src/contracts/current/test/Whitelist/Whitelist.sol @@ -114,6 +114,8 @@ contract Whitelist is } /// @dev Verifies signer is same as signer of current Ethereum transaction. + /// NOTE: This function can currently be used to validate signatures coming from outside of this contract. + /// Extra safety checks can be added for a production contract. /// @param signer Address that should have signed the given hash. /// @param signature Proof of signing. /// @return Validity of order signature. -- cgit v1.2.3 From 101e9be7b928493efe96c6a3c05641c68e30a02e Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Fri, 25 May 2018 15:04:44 -0700 Subject: Change names of signature types --- .../current/protocol/Exchange/MixinSignatureValidator.sol | 8 ++++---- .../current/protocol/Exchange/mixins/MSignatureValidator.sol | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 4b0b1d02d..fc61112eb 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -132,7 +132,7 @@ contract MixinSignatureValidator is return isValid; // Signed using web3.eth_sign - } else if (signatureType == SignatureType.Ecrecover) { + } else if (signatureType == SignatureType.EthSign) { require( signature.length == 65, INVALID_SIGNATURE_LENGTH @@ -165,9 +165,9 @@ contract MixinSignatureValidator is isValid = signer == msg.sender; return isValid; - // Signature verified by signer contract. - // If used with an order, the maker of the order is the signer contract. - } else if (signatureType == SignatureType.Signer) { + // Signature verified by wallet contract. + // If used with an order, the maker of the order is the wallet contract. + } else if (signatureType == SignatureType.Wallet) { isValid = ISigner(signer).isValidSignature(hash, signature); return isValid; diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol index dc08c6d27..7eed453ff 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MSignatureValidator.sol @@ -28,9 +28,9 @@ contract MSignatureValidator is Illegal, // 0x00, default value Invalid, // 0x01 EIP712, // 0x02 - Ecrecover, // 0x03 + EthSign, // 0x03 Caller, // 0x04 - Signer, // 0x05 + Wallet, // 0x05 Validator, // 0x06 PreSigned, // 0x07 Trezor // 0x08 -- cgit v1.2.3 From d625b65a095381c3fa8dd3192ee2115ee739cc07 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 29 May 2018 09:03:23 -0700 Subject: Make preSigned and allowedValidators mappings public --- .../contracts/current/protocol/Exchange/MixinSignatureValidator.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index fc61112eb..7f46766d6 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -33,10 +33,10 @@ contract MixinSignatureValidator is { // Mapping of hash => signer => signed - mapping (bytes32 => mapping (address => bool)) preSigned; + mapping (bytes32 => mapping (address => bool)) public preSigned; // Mapping of signer => validator => approved - mapping (address => mapping (address => bool)) allowedValidators; + mapping (address => mapping (address => bool)) public allowedValidators; /// @dev Approves a hash on-chain using any valid signature type. /// After presigning a hash, the preSign signature type will become valid for that hash and signer. -- cgit v1.2.3 From 8f2fd9b603f74aa8a44dc0e8c3b845d497f9b279 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 29 May 2018 11:22:57 -0700 Subject: Rename Signer to Wallet, rename GAS_ESTIMATE to GAS_LIMIT --- .../protocol/Exchange/MixinSignatureValidator.sol | 4 +-- .../protocol/Exchange/interfaces/ISigner.sol | 34 ---------------------- .../protocol/Exchange/interfaces/IWallet.sol | 34 ++++++++++++++++++++++ packages/contracts/src/utils/web3_wrapper.ts | 2 +- 4 files changed, 37 insertions(+), 37 deletions(-) delete mode 100644 packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISigner.sol create mode 100644 packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IWallet.sol (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 7f46766d6..7f4e20b5b 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -20,7 +20,7 @@ pragma solidity ^0.4.24; import "./mixins/MSignatureValidator.sol"; import "./mixins/MTransactions.sol"; -import "./interfaces/ISigner.sol"; +import "./interfaces/IWallet.sol"; import "./interfaces/IValidator.sol"; import "./libs/LibExchangeErrors.sol"; import "../../utils/LibBytes/LibBytes.sol"; @@ -168,7 +168,7 @@ contract MixinSignatureValidator is // Signature verified by wallet contract. // If used with an order, the maker of the order is the wallet contract. } else if (signatureType == SignatureType.Wallet) { - isValid = ISigner(signer).isValidSignature(hash, signature); + isValid = IWallet(signer).isValidSignature(hash, signature); return isValid; // Signature verified by validator contract. diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISigner.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISigner.sol deleted file mode 100644 index 0186ad490..000000000 --- a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISigner.sol +++ /dev/null @@ -1,34 +0,0 @@ -/* - - Copyright 2018 ZeroEx Intl. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. - -*/ - -pragma solidity ^0.4.24; - -contract ISigner { - - /// @dev Verifies that a signature is valid. - /// @param hash Message hash that is signed. - /// @param signature Proof of signing. - /// @return Validity of order signature. - function isValidSignature( - bytes32 hash, - bytes signature - ) - external - view - returns (bool isValid); -} diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IWallet.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IWallet.sol new file mode 100644 index 000000000..c86a2c057 --- /dev/null +++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/IWallet.sol @@ -0,0 +1,34 @@ +/* + + Copyright 2018 ZeroEx Intl. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +*/ + +pragma solidity ^0.4.24; + +contract IWallet { + + /// @dev Verifies that a signature is valid. + /// @param hash Message hash that is signed. + /// @param signature Proof of signing. + /// @return Validity of order signature. + function isValidSignature( + bytes32 hash, + bytes signature + ) + external + view + returns (bool isValid); +} diff --git a/packages/contracts/src/utils/web3_wrapper.ts b/packages/contracts/src/utils/web3_wrapper.ts index 02595506b..4b8512222 100644 --- a/packages/contracts/src/utils/web3_wrapper.ts +++ b/packages/contracts/src/utils/web3_wrapper.ts @@ -7,7 +7,7 @@ import { coverage } from './coverage'; export const txDefaults = { from: devConstants.TESTRPC_FIRST_ADDRESS, - gas: devConstants.GAS_ESTIMATE, + gas: devConstants.GAS_LIMIT, }; const providerConfigs = { shouldUseInProcessGanache: true }; export const provider = web3Factory.getRpcProvider(providerConfigs); -- cgit v1.2.3 From 79e7c44884f81f12733d555314c54d4c912f0e88 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 30 May 2018 17:52:37 -0700 Subject: Check length before accessing indices, add awaitTransactionSuccess where needed, and rename function --- .../src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol | 10 ++++++---- .../src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol | 10 ++++++---- .../current/protocol/Exchange/MixinSignatureValidator.sol | 4 ++-- .../protocol/Exchange/interfaces/ISignatureValidator.sol | 4 ++-- 4 files changed, 16 insertions(+), 12 deletions(-) (limited to 'packages/contracts/src') diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol index 7c25c9770..79824fbbb 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol @@ -53,16 +53,18 @@ contract ERC20Proxy is { // Data must be intended for this proxy. uint256 length = assetMetadata.length; + + require( + length == 21, + INVALID_METADATA_LENGTH + ); + require( uint8(assetMetadata[length - 1]) == PROXY_ID, PROXY_ID_MISMATCH ); // Decode metadata. - require( - length == 21, - INVALID_METADATA_LENGTH - ); address token = readAddress(assetMetadata, 0); // Transfer tokens. diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol index 59045f73a..ace3570bc 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol @@ -53,6 +53,12 @@ contract ERC721Proxy is { // Data must be intended for this proxy. uint256 length = assetMetadata.length; + + require( + length == 53, + INVALID_METADATA_LENGTH + ); + require( uint8(assetMetadata[length - 1]) == PROXY_ID, PROXY_ID_MISMATCH @@ -65,10 +71,6 @@ contract ERC721Proxy is ); // Decode metadata - require( - length == 53, - INVALID_METADATA_LENGTH - ); address token = readAddress(assetMetadata, 0); uint256 tokenId = readUint256(assetMetadata, 20); diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol index 7f4e20b5b..d40974e5f 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinSignatureValidator.sol @@ -56,10 +56,10 @@ contract MixinSignatureValidator is preSigned[hash][signer] = true; } - /// @dev Approves a Validator contract to verify signatures on signer's behalf. + /// @dev Approves/unnapproves a Validator contract to verify signatures on signer's behalf. /// @param validator Address of Validator contract. /// @param approval Approval or disapproval of Validator contract. - function approveSignatureValidator( + function setSignatureValidatorApproval( address validator, bool approval ) diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol index 72f15a2a4..26e360c91 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/interfaces/ISignatureValidator.sol @@ -31,10 +31,10 @@ contract ISignatureValidator { ) external; - /// @dev Approves a Validator contract to verify signatures on signer's behalf. + /// @dev Approves/unnapproves a Validator contract to verify signatures on signer's behalf. /// @param validator Address of Validator contract. /// @param approval Approval or disapproval of Validator contract. - function approveSignatureValidator( + function setSignatureValidatorApproval( address validator, bool approval ) -- cgit v1.2.3