diff options
author | Greg Hysen <greg.hysen@gmail.com> | 2018-06-02 02:54:20 +0800 |
---|---|---|
committer | Greg Hysen <greg.hysen@gmail.com> | 2018-06-08 06:38:48 +0800 |
commit | 3ed13150e106c19563c8e9b06621be3d44d66b6c (patch) | |
tree | ad7b57063d01ce23b3300e6320469383c9103c01 /packages/contracts | |
parent | f03e5c6bd12c88fffbad324fd7493d3acedea0aa (diff) | |
download | dexon-0x-contracts-3ed13150e106c19563c8e9b06621be3d44d66b6c.tar dexon-0x-contracts-3ed13150e106c19563c8e9b06621be3d44d66b6c.tar.gz dexon-0x-contracts-3ed13150e106c19563c8e9b06621be3d44d66b6c.tar.bz2 dexon-0x-contracts-3ed13150e106c19563c8e9b06621be3d44d66b6c.tar.lz dexon-0x-contracts-3ed13150e106c19563c8e9b06621be3d44d66b6c.tar.xz dexon-0x-contracts-3ed13150e106c19563c8e9b06621be3d44d66b6c.tar.zst dexon-0x-contracts-3ed13150e106c19563c8e9b06621be3d44d66b6c.zip |
Style audit for proxies + libmem + libbytes
Diffstat (limited to 'packages/contracts')
8 files changed, 72 insertions, 50 deletions
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol index 54cbeb963..96950f1cd 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol @@ -47,7 +47,7 @@ contract ERC20Proxy is ) internal { - // Decode proxy data. + // Decode asset data. ( uint8 proxyId, address token diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol index 58c23b03b..102064c15 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol @@ -48,7 +48,7 @@ contract ERC721Proxy is ) internal { - // Decode proxy data. + // Decode asset data. ( uint8 proxyId, address token, @@ -118,7 +118,7 @@ contract ERC721Proxy is INVALID_ASSET_DATA_LENGTH ); - // Decode asset data + // Decode asset data. proxyId = uint8(assetData[0]); token = readAddress(assetData, 1); tokenId = readUint256(assetData, 21); diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol index 0bf11b03b..f009a6a71 100644 --- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol +++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol @@ -155,7 +155,6 @@ contract TestLibBytes is return b; } -======= /// @dev Reads the first 4 bytes from a byte array of arbitrary length. /// @param b Byte array to read first 4 bytes from. /// @return First 4 bytes of data. @@ -168,6 +167,10 @@ contract TestLibBytes is return result; } + /// @dev Reads nested bytes from a specific position. + /// @param b Byte array containing nested bytes. + /// @param index Index of nested bytes. + /// @return result Nested bytes. function publicReadBytes( bytes memory b, uint256 index) @@ -179,7 +182,11 @@ contract TestLibBytes is return result; } - + /// @dev Inserts bytes at a specific position in a byte array. + /// @param b Byte array to insert <input> into. + /// @param index Index in byte array of <input>. + /// @param input bytes to insert. + /// @return b Updated input byte array function publicWriteBytes( bytes memory b, uint256 index, diff --git a/packages/contracts/src/contracts/current/test/TestLibMem/TestLibMem.sol b/packages/contracts/src/contracts/current/test/TestLibMem/TestLibMem.sol index 64bc182f4..076bee24c 100644 --- a/packages/contracts/src/contracts/current/test/TestLibMem/TestLibMem.sol +++ b/packages/contracts/src/contracts/current/test/TestLibMem/TestLibMem.sol @@ -23,8 +23,15 @@ import "../../utils/LibMem/LibMem.sol"; contract TestLibMem is LibMem { + + /// @dev Copies a block of memory from one location to another. + /// @param mem Memory contents we want to apply memcpy to + /// @param dest Destination offset into <mem>. + /// @param source Source offset into <mem>. + /// @param length Length of bytes to copy from <source> to <dest> + /// @return mem Memory contents after calling memcpy. function testMemcpy( - bytes mem, ///< Memory contents we want to apply memcpy to + bytes mem, uint256 dest, uint256 source, uint256 length @@ -36,13 +43,13 @@ contract TestLibMem is // Sanity check. Overflows are not checked. require(source + length <= mem.length); require(dest + length <= mem.length); - + // Get pointer to memory contents uint256 offset = getMemAddress(mem) + 32; - + // Execute memcpy adjusted for memory array location memcpy(offset + dest, offset + source, length); - + // Return modified memory contents return mem; } diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol index 6351f1a46..5610c47b3 100644 --- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol +++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol @@ -268,7 +268,6 @@ contract LibBytes is writeBytes32(b, index, bytes32(input)); } -======= /// @dev Reads the first 4 bytes from a byte array of arbitrary length. /// @param b Byte array to read first 4 bytes from. /// @return First 4 bytes of data. @@ -287,10 +286,10 @@ contract LibBytes is return result; } - /// @dev Reads a uint256 value from a position in a byte array. - /// @param b Byte array containing a uint256 value. - /// @param index Index in byte array of uint256 value. - /// @return uint256 value from byte array. + /// @dev Reads nested bytes from a specific position. + /// @param b Byte array containing nested bytes. + /// @param index Index of nested bytes. + /// @return result Nested bytes. function readBytes( bytes memory b, uint256 index @@ -321,10 +320,10 @@ contract LibBytes is return result; } - /// @dev Writes a uint256 into a specific position in a byte array. + /// @dev Inserts bytes at a specific position in a byte array. /// @param b Byte array to insert <input> into. /// @param index Index in byte array of <input>. - /// @param input uint256 to put into byte array. + /// @param input bytes to insert. function writeBytes( bytes memory b, uint256 index, diff --git a/packages/contracts/src/contracts/current/utils/LibMem/LibMem.sol b/packages/contracts/src/contracts/current/utils/LibMem/LibMem.sol index 500044500..960850725 100644 --- a/packages/contracts/src/contracts/current/utils/LibMem/LibMem.sol +++ b/packages/contracts/src/contracts/current/utils/LibMem/LibMem.sol @@ -18,23 +18,27 @@ pragma solidity ^0.4.24; -contract LibMem { +contract LibMem +{ + /// @dev Gets the memory address for a byte array. + /// @param input Byte array to lookup. + /// @return memoryAddress Memory address of byte array. function getMemAddress(bytes memory input) internal pure - returns (uint256 address_) + returns (uint256 memoryAddress) { assembly { - address_ := input + memoryAddress := input } - return address_; + return memoryAddress; } /// @dev Copies `length` bytes from memory location `source` to `dest`. - /// @param dest memory address to copy bytes to - /// @param source memory address to copy bytes from - /// @param length number of bytes to copy + /// @param dest memory address to copy bytes to. + /// @param source memory address to copy bytes from. + /// @param length number of bytes to copy. function memcpy( uint256 dest, uint256 source, diff --git a/packages/contracts/test/asset_proxy/decoder.ts b/packages/contracts/test/asset_proxy/decoder.ts index 8c1253d5c..e395c04c1 100644 --- a/packages/contracts/test/asset_proxy/decoder.ts +++ b/packages/contracts/test/asset_proxy/decoder.ts @@ -19,7 +19,7 @@ chaiSetup.configure(); const expect = chai.expect; const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); -describe('LibAssetProxyDecoder', () => { +describe('TestAssetDataDecoders', () => { let owner: string; let testAssetProxyDecoder: TestAssetDataDecodersContract; let testAddress: string; @@ -43,8 +43,8 @@ describe('LibAssetProxyDecoder', () => { await blockchainLifecycle.revertAsync(); }); - describe('LibAssetProxyDecoder', () => { - it('should correctly decode ERC20 proxy data)', async () => { + describe('Asset Data Decoders', () => { + it('should correctly decode ERC20 asset data)', async () => { const encodedAssetData = assetProxyUtils.encodeERC20AssetData(testAddress); const expectedDecodedAssetData = assetProxyUtils.decodeERC20AssetData(encodedAssetData); let decodedAssetProxyId: number; @@ -56,7 +56,7 @@ describe('LibAssetProxyDecoder', () => { expect(decodedTokenAddress).to.be.equal(expectedDecodedAssetData.tokenAddress); }); - it('should correctly decode ERC721 proxy data', async () => { + it('should correctly decode ERC721 asset data', async () => { const tokenId = ZeroEx.generatePseudoRandomSalt(); const encodedAssetData = assetProxyUtils.encodeERC721AssetData(testAddress, tokenId); const expectedDecodedAssetData = assetProxyUtils.decodeERC721AssetData(encodedAssetData); @@ -76,25 +76,26 @@ describe('LibAssetProxyDecoder', () => { expect(decodedData).to.be.equal(expectedDecodedAssetData.data); }); - it('should correctly decode ERC721 proxy data with receiver data', async () => { + it('should correctly decode ERC721 asset data with receiver data', async () => { const tokenId = ZeroEx.generatePseudoRandomSalt(); - const data = ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt())) + 'FFFF'; - const encodedAssetData = assetProxyUtils.encodeERC721AssetData(testAddress, tokenId, data); + const receiverData = + ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt())) + 'FFFF'; + const encodedAssetData = assetProxyUtils.encodeERC721AssetData(testAddress, tokenId, receiverData); const expectedDecodedAssetData = assetProxyUtils.decodeERC721AssetData(encodedAssetData); let decodedAssetProxyId: number; let decodedTokenAddress: string; let decodedTokenId: BigNumber; - let decodedData: string; + let decodedReceiverData: string; [ decodedAssetProxyId, decodedTokenAddress, decodedTokenId, - decodedData, + decodedReceiverData, ] = await testAssetProxyDecoder.publicDecodeERC721Data.callAsync(encodedAssetData); expect(decodedAssetProxyId).to.be.equal(expectedDecodedAssetData.assetProxyId); expect(decodedTokenAddress).to.be.equal(expectedDecodedAssetData.tokenAddress); expect(decodedTokenId).to.be.bignumber.equal(expectedDecodedAssetData.tokenId); - expect(decodedData).to.be.equal(expectedDecodedAssetData.data); + expect(decodedReceiverData).to.be.equal(expectedDecodedAssetData.data); }); }); }); diff --git a/packages/contracts/test/asset_proxy/proxies.ts b/packages/contracts/test/asset_proxy/proxies.ts index f44c44045..e8c598935 100644 --- a/packages/contracts/test/asset_proxy/proxies.ts +++ b/packages/contracts/test/asset_proxy/proxies.ts @@ -106,7 +106,7 @@ describe('Asset Transfer Proxies', () => { describe('Transfer Proxy - ERC20', () => { describe('transferFrom', () => { it('should successfully transfer tokens', async () => { - // Construct metadata for ERC20 proxy + // Construct ERC20 asset data const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address); // Perform a transfer from makerAddress to takerAddress const erc20Balances = await erc20Wrapper.getBalancesAsync(); @@ -132,7 +132,7 @@ describe('Asset Transfer Proxies', () => { }); it('should do nothing if transferring 0 amount of a token', async () => { - // Construct metadata for ERC20 proxy + // Construct ERC20 asset data const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address); // Perform a transfer from makerAddress to takerAddress const erc20Balances = await erc20Wrapper.getBalancesAsync(); @@ -158,7 +158,7 @@ describe('Asset Transfer Proxies', () => { }); it('should throw if allowances are too low', async () => { - // Construct metadata for ERC20 proxy + // Construct ERC20 asset data const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address); // Create allowance less than transfer amount. Set allowance on proxy. const allowance = new BigNumber(0); @@ -182,7 +182,7 @@ describe('Asset Transfer Proxies', () => { }); it('should throw if requesting address is not authorized', async () => { - // Construct metadata for ERC20 proxy + // Construct ERC20 asset data const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(10); @@ -258,7 +258,7 @@ describe('Asset Transfer Proxies', () => { describe('Transfer Proxy - ERC721', () => { describe('transferFrom', () => { it('should successfully transfer tokens', async () => { - // Construct metadata for ERC721 proxy + // Construct ERC721 asset data const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId); // Verify pre-condition const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId); @@ -281,7 +281,7 @@ describe('Asset Transfer Proxies', () => { }); it('should not call onERC721Received when transferring to a smart contract without receiver data', async () => { - // Construct metadata for ERC721 proxy + // Construct ERC721 asset data const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId); // Verify pre-condition const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId); @@ -310,12 +310,14 @@ describe('Asset Transfer Proxies', () => { }); it('should call onERC721Received when transferring to a smart contract with receiver data', async () => { - // Construct metadata for ERC721 proxy - const data = ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt())); + // Construct ERC721 asset data + const receiverData = ethUtil.bufferToHex( + assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt()), + ); const encodedAssetData = assetProxyUtils.encodeERC721AssetData( erc721Token.address, erc721MakerTokenId, - data, + receiverData, ); // Verify pre-condition const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId); @@ -340,19 +342,21 @@ describe('Asset Transfer Proxies', () => { const tokenReceivedLog = tx.logs[0] as LogWithDecodedArgs<TokenReceivedContractEventArgs>; expect(tokenReceivedLog.args.from).to.be.equal(makerAddress); expect(tokenReceivedLog.args.tokenId).to.be.bignumber.equal(erc721MakerTokenId); - expect(tokenReceivedLog.args.data).to.be.equal(data); + expect(tokenReceivedLog.args.data).to.be.equal(receiverData); // Verify transfer was successful const newOwnerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId); expect(newOwnerMakerAsset).to.be.bignumber.equal(erc721Receiver.address); }); it('should throw if there is receiver data but contract does not have onERC721Received', async () => { - // Construct metadata for ERC721 proxy - const data = ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt())); + // Construct ERC721 asset data + const receiverData = ethUtil.bufferToHex( + assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt()), + ); const encodedAssetData = assetProxyUtils.encodeERC721AssetData( erc721Token.address, erc721MakerTokenId, - data, + receiverData, ); // Verify pre-condition const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId); @@ -372,7 +376,7 @@ describe('Asset Transfer Proxies', () => { }); it('should throw if transferring 0 amount of a token', async () => { - // Construct metadata for ERC721 proxy + // Construct ERC721 asset data const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId); // Verify pre-condition const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId); @@ -391,7 +395,7 @@ describe('Asset Transfer Proxies', () => { }); it('should throw if transferring > 1 amount of a token', async () => { - // Construct metadata for ERC721 proxy + // Construct ERC721 asset data const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId); // Verify pre-condition const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId); @@ -410,7 +414,7 @@ describe('Asset Transfer Proxies', () => { }); it('should throw if allowances are too low', async () => { - // Construct metadata for ERC721 proxy + // Construct ERC721 asset data const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId); // Remove transfer approval for makerAddress. await web3Wrapper.awaitTransactionSuccessAsync( @@ -429,7 +433,7 @@ describe('Asset Transfer Proxies', () => { }); it('should throw if requesting address is not authorized', async () => { - // Construct metadata for ERC721 proxy + // Construct ERC721 asset data const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(1); |