Not following 0x specification for 1155 assetdata encoding.

[MicahZoltu]

https://github.com/AugurProject/augur/blob/201c54b5e2c18e5e3d6bec2156b7afcf6eefde28/packages/augur-core/source/contracts/trading/ZeroXTrade.sol#L305-L312

The specification for encoding this data says that it should encode 5 parameters using standard ABI encoding. It then indicates that it is acceptable to include additional data on the end of the encoded data block if extra data is desired. This code however doesn't do that and instead encodes 6 parameters, where the last parameter is kycToken.

Someone writing code like this will have a bad day when working with Augur assets:

// we only need token IDs, so don't bother decoding the whole thing
const tokenIdsLength = assetData.slice(2 + 64*4)

Recommend changing the encoding to something like:

_assetData = abi.encodeWithSelector( 
    ERC1155_PROXY_ID, 
    address(this), 
    _tokenIds, 
    _tokenValues, 
    _callbackData
);
_assetData = _assetData.append(_kycToken) // library method for working with `bytes`

[MicahZoltu]

For a bit more clarity: Current data layout:

<selector> // 4 bytes
<token> // 32 bytes
<tokenIdsOffset> // 32 bytes
<tokenValuesOffest> // 32 bytes
<callbackDataOffset> // 32 bytes
<kycToken> // 32 bytes
<tokenIds> // length prefixed bytes
<tokenValues> // length prefixed bytes
<callbackData> // length prefixed bytes

Correct data layout:

<selector> // 4 bytes
<token> // 32 bytes
<tokenIdsOffset> // 32 bytes
<tokenValuesOffest> // 32 bytes
<callbackDataOffset> // 32 bytes
<tokenIds> // length prefixed bytes
<tokenValues> // length prefixed bytes
<callbackData> // length prefixed bytes
<kycToken> // 32 bytes

Note that kycToken was moved to the end from the middle.

[epheph]

This is not a valid way to decode the tokenIds, regardless of how additional arguments are encoded in there:

// we only need token IDs, so don't bother decoding the whole thing
const tokenIdsLength = assetData.slice(2 + 64*4)

There is no guarantee they will be at that location, the encoder could have easily swapped tokenIds and tokenValues offsets, regardless of an extra kycToken being in there. The question is really "do you put the extra data in an interior unaddressed space or unaddressed space at the end?". Probably more correct to do it at the end, as you stated, but more for cleanliness than security.

[nuevoalex]

Did that code get committed already? If not could that comment be in the PR for it if there is one?

[epheph]

I think the code @MicahZoltu wrote in the PR description was speculative about future issues that could arise from the placement of kycToken in the encoded data.