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') 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