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 --- .../current/protocol/AssetProxy/ERC20Proxy.sol | 10 +++--- .../current/protocol/AssetProxy/ERC721Proxy.sol | 10 +++--- .../protocol/Exchange/MixinSignatureValidator.sol | 4 +-- .../Exchange/interfaces/ISignatureValidator.sol | 4 +-- packages/contracts/test/exchange/transactions.ts | 40 +++++++++++++++------- 5 files changed, 43 insertions(+), 25 deletions(-) (limited to 'packages') 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 ) diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts index 2b2236e2e..6b3083ae5 100644 --- a/packages/contracts/test/exchange/transactions.ts +++ b/packages/contracts/test/exchange/transactions.ts @@ -220,9 +220,11 @@ describe('Exchange transactions', () => { exchange.address, ); const isApproved = true; - await exchange.approveSignatureValidator.sendTransactionAsync(whitelist.address, isApproved, { - from: takerAddress, - }); + await web3Wrapper.awaitTransactionSuccessAsync( + await exchange.approveSignatureValidator.sendTransactionAsync(whitelist.address, isApproved, { + from: takerAddress, + }), + ); const defaultOrderParams = { ...constants.STATIC_ORDER_PARAMS, senderAddress: whitelist.address, @@ -242,7 +244,9 @@ describe('Exchange transactions', () => { it('should revert if maker has not been whitelisted', async () => { const isApproved = true; - await whitelist.updateWhitelistStatus.sendTransactionAsync(takerAddress, isApproved, { from: owner }); + await web3Wrapper.awaitTransactionSuccessAsync( + await whitelist.updateWhitelistStatus.sendTransactionAsync(takerAddress, isApproved, { from: owner }), + ); const orderStruct = orderUtils.getOrderStruct(signedOrder); const takerAssetFillAmount = signedOrder.takerAssetAmount; @@ -260,7 +264,9 @@ describe('Exchange transactions', () => { it('should revert if taker has not been whitelisted', async () => { const isApproved = true; - await whitelist.updateWhitelistStatus.sendTransactionAsync(makerAddress, isApproved, { from: owner }); + await web3Wrapper.awaitTransactionSuccessAsync( + await whitelist.updateWhitelistStatus.sendTransactionAsync(makerAddress, isApproved, { from: owner }), + ); const orderStruct = orderUtils.getOrderStruct(signedOrder); const takerAssetFillAmount = signedOrder.takerAssetAmount; @@ -278,19 +284,27 @@ describe('Exchange transactions', () => { it('should fill the order if maker and taker have been whitelisted', async () => { const isApproved = true; - await whitelist.updateWhitelistStatus.sendTransactionAsync(makerAddress, isApproved, { from: owner }); - await whitelist.updateWhitelistStatus.sendTransactionAsync(takerAddress, isApproved, { from: owner }); + await web3Wrapper.awaitTransactionSuccessAsync( + await whitelist.updateWhitelistStatus.sendTransactionAsync(makerAddress, isApproved, { from: owner }), + ); + + await web3Wrapper.awaitTransactionSuccessAsync( + await whitelist.updateWhitelistStatus.sendTransactionAsync(takerAddress, isApproved, { from: owner }), + ); const orderStruct = orderUtils.getOrderStruct(signedOrder); const takerAssetFillAmount = signedOrder.takerAssetAmount; const salt = generatePseudoRandomSalt(); - await whitelist.fillOrderIfWhitelisted.sendTransactionAsync( - orderStruct, - takerAssetFillAmount, - salt, - signedOrder.signature, - { from: takerAddress }, + await web3Wrapper.awaitTransactionSuccessAsync( + await whitelist.fillOrderIfWhitelisted.sendTransactionAsync( + orderStruct, + takerAssetFillAmount, + salt, + signedOrder.signature, + { from: takerAddress }, + ), ); + const newBalances = await erc20Wrapper.getBalancesAsync(); const makerAssetFillAmount = signedOrder.makerAssetAmount; -- cgit v1.2.3