From dcb12b6ad6485f987d574ee3b20bf24aaf7a7d40 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 29 Aug 2018 14:05:28 -0700 Subject: Update AssetProxyOwner to 0.4.24 --- .../protocol/AssetProxyOwner/AssetProxyOwner.sol | 49 +++++++--------------- .../TestAssetProxyOwner/TestAssetProxyOwner.sol | 23 +++------- .../contracts/test/multisig/asset_proxy_owner.ts | 19 --------- 3 files changed, 21 insertions(+), 70 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 7da3d87e2..bb02d8489 100644 --- a/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol +++ b/packages/contracts/src/2.0.0/protocol/AssetProxyOwner/AssetProxyOwner.sol @@ -16,15 +16,18 @@ */ -pragma solidity 0.4.10; +pragma solidity 0.4.24; import "../../multisig/MultiSigWalletWithTimeLock.sol"; +import "../../utils/LibBytes/LibBytes.sol"; contract AssetProxyOwner is MultiSigWalletWithTimeLock { + using LibBytes for bytes; + event AssetProxyRegistration(address assetProxyContract, bool isRegistered); // Mapping of AssetProxy contract address => @@ -37,8 +40,14 @@ contract AssetProxyOwner is /// on an approved AssetProxy contract. modifier validRemoveAuthorizedAddressAtIndexTx(uint256 transactionId) { Transaction storage tx = transactions[transactionId]; - require(isAssetProxyRegistered[tx.destination]); - require(readBytes4(tx.data, 0) == REMOVE_AUTHORIZED_ADDRESS_AT_INDEX_SELECTOR); + require( + isAssetProxyRegistered[tx.destination], + "UNREGISTERED_ASSET_PROXY" + ); + require( + tx.data.readBytes4(0) == REMOVE_AUTHORIZED_ADDRESS_AT_INDEX_SELECTOR, + "INVALID_FUNCTION_SELECTOR" + ); _; } @@ -48,7 +57,7 @@ contract AssetProxyOwner is /// @param _assetProxyContracts Array of AssetProxy contract addresses. /// @param _required Number of required confirmations. /// @param _secondsTimeLocked Duration needed after a transaction is confirmed and before it becomes executable, in seconds. - function AssetProxyOwner( + constructor ( address[] memory _owners, address[] memory _assetProxyContracts, uint256 _required, @@ -74,7 +83,7 @@ contract AssetProxyOwner is notNull(assetProxyContract) { isAssetProxyRegistered[assetProxyContract] = isRegistered; - AssetProxyRegistration(assetProxyContract, isRegistered); + emit AssetProxyRegistration(assetProxyContract, isRegistered); } /// @dev Allows execution of `removeAuthorizedAddressAtIndex` without time lock. @@ -89,36 +98,10 @@ contract AssetProxyOwner is tx.executed = true; // solhint-disable-next-line avoid-call-value if (tx.destination.call.value(tx.value)(tx.data)) - Execution(transactionId); + emit Execution(transactionId); else { - ExecutionFailure(transactionId); + emit ExecutionFailure(transactionId); 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); - - // Arrays are prefixed by a 32 byte length field - index += 32; - - // Read the bytes4 from array memory - assembly { - result := mload(add(b, index)) - // 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 75e782d43..38ec42a72 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.10; +pragma solidity 0.4.24; import "../../protocol/AssetProxyOwner/AssetProxyOwner.sol"; @@ -26,7 +26,7 @@ contract TestAssetProxyOwner is AssetProxyOwner { - function TestAssetProxyOwner( + constructor ( address[] memory _owners, address[] memory _assetProxyContracts, uint256 _required, @@ -38,6 +38,7 @@ contract TestAssetProxyOwner is function testValidRemoveAuthorizedAddressAtIndexTx(uint256 id) public + view validRemoveAuthorizedAddressAtIndexTx(id) returns (bool) { @@ -50,23 +51,9 @@ contract TestAssetProxyOwner is /// @return Successful if data is a call to `removeAuthorizedAddressAtIndex`. function isFunctionRemoveAuthorizedAddressAtIndex(bytes memory data) public + pure returns (bool) { - 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; + return data.readBytes4(0) == REMOVE_AUTHORIZED_ADDRESS_AT_INDEX_SELECTOR; } } diff --git a/packages/contracts/test/multisig/asset_proxy_owner.ts b/packages/contracts/test/multisig/asset_proxy_owner.ts index 9515941ff..2f626daef 100644 --- a/packages/contracts/test/multisig/asset_proxy_owner.ts +++ b/packages/contracts/test/multisig/asset_proxy_owner.ts @@ -148,25 +148,6 @@ 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; -- cgit v1.2.3