Merge pull request #837 from 0xProject/fix/contracts/assetProxyOwnerVersion

Use 0.4.10 in AssetProxyOwner

7 files changed, 76 insertions, 17 deletions

diff --git a/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol b/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol
index e7cf4ab5c..8b7333646 100644
--- a/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol
+++ b/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol
@@ -16,16 +16,14 @@
 
 */
 
-pragma solidity ^0.4.10;
+pragma solidity 0.4.10;
 
 import "../../multisig/MultiSigWalletWithTimeLock.sol";
-import "../../utils/LibBytes/LibBytes.sol";
 
 
 contract AssetProxyOwner is
     MultiSigWalletWithTimeLock
 {
-    using LibBytes for bytes;
 
     event AssetProxyRegistration(address assetProxyContract, bool isRegistered);
 
@@ -40,7 +38,7 @@ contract AssetProxyOwner is
     modifier validRemoveAuthorizedAddressAtIndexTx(uint256 transactionId) {
         Transaction storage tx = transactions[transactionId];
         require(isAssetProxyRegistered[tx.destination]);
-        require(tx.data.readBytes4(0) == REMOVE_AUTHORIZED_ADDRESS_AT_INDEX_SELECTOR);
+        require(readBytes4(tx.data, 0) == REMOVE_AUTHORIZED_ADDRESS_AT_INDEX_SELECTOR);
         _;
     }
 
@@ -97,4 +95,25 @@ contract AssetProxyOwner is
             tx.executed = false;
         }
     }
+
+    /// @dev Reads an unpadded bytes4 value from a position in a byte array.
+    /// @param b Byte array containing a bytes4 value.
+    /// @param index Index in byte array of bytes4 value.
+    /// @return bytes4 value from byte array.
+    function readBytes4(
+        bytes memory b,
+        uint256 index
+    )
+        internal
+        returns (bytes4 result)
+    {
+        require(b.length >= index + 4);
+        assembly {
+            result := mload(add(b, 32))
+            // Solidity does not require us to clean the trailing bytes.
+            // We do it anyway
+            result := and(result, 0xFFFFFFFF00000000000000000000000000000000000000000000000000000000)
+        }
+        return result;
+    }
 }
diff --git a/packages/contracts/src/2.0.0/test/TestAssetProxyOwner/TestAssetProxyOwner.sol b/packages/contracts/src/2.0.0/test/TestAssetProxyOwner/TestAssetProxyOwner.sol
index d6b6b29f2..75e782d43 100644
--- a/packages/contracts/src/2.0.0/test/TestAssetProxyOwner/TestAssetProxyOwner.sol
+++ b/packages/contracts/src/2.0.0/test/TestAssetProxyOwner/TestAssetProxyOwner.sol
@@ -16,7 +16,7 @@
 
 */
 
-pragma solidity 0.4.24;
+pragma solidity 0.4.10;
 
 import "../../protocol/AssetProxyOwner/AssetProxyOwner.sol";
 
@@ -26,7 +26,7 @@ contract TestAssetProxyOwner is
     AssetProxyOwner
 {
 
-    constructor(
+    function TestAssetProxyOwner(
         address[] memory _owners,
         address[] memory _assetProxyContracts,
         uint256 _required,
@@ -38,7 +38,6 @@ contract TestAssetProxyOwner is
     
     function testValidRemoveAuthorizedAddressAtIndexTx(uint256 id)
         public
-        view
         validRemoveAuthorizedAddressAtIndexTx(id)
         returns (bool)
     {
@@ -51,9 +50,23 @@ contract TestAssetProxyOwner is
     /// @return Successful if data is a call to `removeAuthorizedAddressAtIndex`.
     function isFunctionRemoveAuthorizedAddressAtIndex(bytes memory data)
         public
-        pure
         returns (bool)
     {
-        return data.readBytes4(0) == REMOVE_AUTHORIZED_ADDRESS_AT_INDEX_SELECTOR;
+        return readBytes4(data, 0) == REMOVE_AUTHORIZED_ADDRESS_AT_INDEX_SELECTOR;
+    }
+
+    /// @dev Reads an unpadded bytes4 value from a position in a byte array.
+    /// @param b Byte array containing a bytes4 value.
+    /// @param index Index in byte array of bytes4 value.
+    /// @return bytes4 value from byte array.
+    function publicReadBytes4(
+        bytes memory b,
+        uint256 index
+    )
+        public
+        returns (bytes4 result)
+    {
+        result = readBytes4(b, index);
+        return result;
     }
 }
diff --git a/packages/contracts/src/2.0.0/utils/LibBytes/LibBytes.sol b/packages/contracts/src/2.0.0/utils/LibBytes/LibBytes.sol
index 01d34fa8f..504e950a8 100644
--- a/packages/contracts/src/2.0.0/utils/LibBytes/LibBytes.sol
+++ b/packages/contracts/src/2.0.0/utils/LibBytes/LibBytes.sol
@@ -457,7 +457,8 @@ library LibBytes {
     /// @return bytes4 value from byte array.
     function readBytes4(
         bytes memory b,
-        uint256 index)
+        uint256 index
+    )
         internal
         pure
         returns (bytes4 result)
diff --git a/packages/contracts/test/asset_proxy/proxies.ts b/packages/contracts/test/asset_proxy/proxies.ts
index e1167b156..36e0800d6 100644
--- a/packages/contracts/test/asset_proxy/proxies.ts
+++ b/packages/contracts/test/asset_proxy/proxies.ts
@@ -272,7 +272,7 @@ describe('Asset Transfer Proxies', () => {
                         to: erc721Proxy.address,
                         data,
                         from: exchangeAddress,
-                        gas: constants.TRANSFER_FROM_GAS,
+                        gas: constants.MAX_TRANSFER_FROM_GAS,
                     }),
                 );
                 // Verify that no log was emitted by erc721 receiver
@@ -311,7 +311,7 @@ describe('Asset Transfer Proxies', () => {
                         to: erc721Proxy.address,
                         data,
                         from: exchangeAddress,
-                        gas: constants.TRANSFER_FROM_GAS,
+                        gas: constants.MAX_TRANSFER_FROM_GAS,
                     }),
                 );
                 // Validate log emitted by erc721 receiver
@@ -349,7 +349,7 @@ describe('Asset Transfer Proxies', () => {
                         to: erc721Proxy.address,
                         data,
                         from: exchangeAddress,
-                        gas: constants.TRANSFER_FROM_GAS,
+                        gas: constants.MAX_TRANSFER_FROM_GAS,
                     }),
                     RevertReason.TransferFailed,
                 );
diff --git a/packages/contracts/test/multisig/asset_proxy_owner.ts b/packages/contracts/test/multisig/asset_proxy_owner.ts
index 10fed6815..6a9843153 100644
--- a/packages/contracts/test/multisig/asset_proxy_owner.ts
+++ b/packages/contracts/test/multisig/asset_proxy_owner.ts
@@ -148,6 +148,25 @@ describe('AssetProxyOwner', () => {
         });
     });
 
+    describe('readBytes4', () => {
+        it('should revert if byte array has a length < 4', async () => {
+            const byteArrayLessThan4Bytes = '0x010101';
+            return expectContractCallFailedWithoutReasonAsync(
+                testAssetProxyOwner.publicReadBytes4.callAsync(byteArrayLessThan4Bytes, new BigNumber(0)),
+            );
+        });
+        it('should return the first 4 bytes of a byte array of arbitrary length', async () => {
+            const byteArrayLongerThan32Bytes =
+                '0x0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef';
+            const first4Bytes = await testAssetProxyOwner.publicReadBytes4.callAsync(
+                byteArrayLongerThan32Bytes,
+                new BigNumber(0),
+            );
+            const expectedFirst4Bytes = byteArrayLongerThan32Bytes.slice(0, 10);
+            expect(first4Bytes).to.equal(expectedFirst4Bytes);
+        });
+    });
+
     describe('registerAssetProxy', () => {
         it('should throw if not called by multisig', async () => {
             const isRegistered = true;
@@ -238,8 +257,6 @@ describe('AssetProxyOwner', () => {
             const registerAssetProxySubmitLog = registerAssetProxySubmitRes.logs[0] as LogWithDecodedArgs<
                 AssetProxyOwnerSubmissionEventArgs
             >;
-            const registerAssetProxyTxId = registerAssetProxySubmitLog.args.transactionId;
-            await multiSigWrapper.confirmTransactionAsync(registerAssetProxyTxId, owners[1]);
 
             const addAuthorizedAddressData = erc20Proxy.addAuthorizedAddress.getABIEncodedTransactionData(authorized);
             const erc20AddAuthorizedAddressSubmitRes = await multiSigWrapper.submitTransactionAsync(
@@ -257,9 +274,12 @@ describe('AssetProxyOwner', () => {
             >;
             const erc721AddAuthorizedAddressSubmitLog = erc721AddAuthorizedAddressSubmitRes
                 .logs[0] as LogWithDecodedArgs<AssetProxyOwnerSubmissionEventArgs>;
+
+            const registerAssetProxyTxId = registerAssetProxySubmitLog.args.transactionId;
             const erc20AddAuthorizedAddressTxId = erc20AddAuthorizedAddressSubmitLog.args.transactionId;
             const erc721AddAuthorizedAddressTxId = erc721AddAuthorizedAddressSubmitLog.args.transactionId;
 
+            await multiSigWrapper.confirmTransactionAsync(registerAssetProxyTxId, owners[1]);
             await multiSigWrapper.confirmTransactionAsync(erc20AddAuthorizedAddressTxId, owners[1]);
             await multiSigWrapper.confirmTransactionAsync(erc721AddAuthorizedAddressTxId, owners[1]);
             await increaseTimeAndMineBlockAsync(SECONDS_TIME_LOCKED.toNumber());
diff --git a/packages/contracts/test/utils/constants.ts b/packages/contracts/test/utils/constants.ts
index 7f3ad62e1..e8995f9d6 100644
--- a/packages/contracts/test/utils/constants.ts
+++ b/packages/contracts/test/utils/constants.ts
@@ -24,9 +24,10 @@ export const constants = {
     // ensure we always use the minimum interval.
     AWAIT_TRANSACTION_MINED_MS: 0,
     MAX_ETHERTOKEN_WITHDRAW_GAS: 43000,
+    MAX_EXECUTE_TRANSACTION_GAS: 1000000,
     MAX_TOKEN_TRANSFERFROM_GAS: 80000,
     MAX_TOKEN_APPROVE_GAS: 60000,
-    TRANSFER_FROM_GAS: 150000,
+    MAX_TRANSFER_FROM_GAS: 150000,
     DUMMY_TOKEN_NAME: '',
     DUMMY_TOKEN_SYMBOL: '',
     DUMMY_TOKEN_DECIMALS: new BigNumber(18),
diff --git a/packages/contracts/test/utils/multi_sig_wrapper.ts b/packages/contracts/test/utils/multi_sig_wrapper.ts
index 6e7746dfc..2024c177d 100644
--- a/packages/contracts/test/utils/multi_sig_wrapper.ts
+++ b/packages/contracts/test/utils/multi_sig_wrapper.ts
@@ -6,6 +6,7 @@ import * as _ from 'lodash';
 import { AssetProxyOwnerContract } from '../../generated_contract_wrappers/asset_proxy_owner';
 import { MultiSigWalletContract } from '../../generated_contract_wrappers/multi_sig_wallet';
 
+import { constants } from './constants';
 import { LogDecoder } from './log_decoder';
 
 export class MultiSigWrapper {
@@ -36,7 +37,10 @@ export class MultiSigWrapper {
         return tx;
     }
     public async executeTransactionAsync(txId: BigNumber, from: string): Promise<TransactionReceiptWithDecodedLogs> {
-        const txHash = await this._multiSig.executeTransaction.sendTransactionAsync(txId, { from });
+        const txHash = await this._multiSig.executeTransaction.sendTransactionAsync(txId, {
+            from,
+            gas: constants.MAX_EXECUTE_TRANSACTION_GAS,
+        });
         const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash);
         return tx;
     }
@@ -48,6 +52,7 @@ export class MultiSigWrapper {
         const txHash = await (this
             ._multiSig as AssetProxyOwnerContract).executeRemoveAuthorizedAddressAtIndex.sendTransactionAsync(txId, {
             from,
+            gas: constants.MAX_EXECUTE_TRANSACTION_GAS,
         });
         const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash);
         return tx;