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 + packages/contracts/test/exchange/transactions.ts | 4 ++++ packages/contracts/test/libraries/lib_bytes.ts | 7 +++---- 7 files changed, 35 insertions(+), 35 deletions(-) (limited to 'packages') 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, diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts index fe6df2f75..294e3aae9 100644 --- a/packages/contracts/test/exchange/transactions.ts +++ b/packages/contracts/test/exchange/transactions.ts @@ -218,6 +218,10 @@ describe('Exchange transactions', () => { txDefaults, exchange.address, ); + const isApproved = true; + await exchange.approveSignatureValidator.sendTransactionAsync(whitelist.address, isApproved, { + from: takerAddress, + }); const defaultOrderParams = { ...constants.STATIC_ORDER_PARAMS, senderAddress: whitelist.address, diff --git a/packages/contracts/test/libraries/lib_bytes.ts b/packages/contracts/test/libraries/lib_bytes.ts index ac654e037..c85cee5e3 100644 --- a/packages/contracts/test/libraries/lib_bytes.ts +++ b/packages/contracts/test/libraries/lib_bytes.ts @@ -62,12 +62,11 @@ describe('LibBytes', () => { describe('deepCopyBytes', () => { const byteArrayLongerThan32BytesLen = (byteArrayLongerThan32Bytes.length - 2) / 2; - it('should throw if length of copy is 0', async () => { + it('should return a byte array of length 0 if len is 0', async () => { const index = new BigNumber(0); const len = new BigNumber(0); - return expect( - libBytes.publicDeepCopyBytes.callAsync(byteArrayLongerThan32Bytes, index, len), - ).to.be.rejectedWith(constants.REVERT); + const copy = await libBytes.publicDeepCopyBytes.callAsync(byteArrayLongerThan32Bytes, index, len); + expect(copy).to.equal(constants.NULL_BYTES); }); it('should throw if start index + length to copy is greater than length of byte array', async () => { -- cgit v1.2.3