QA Report

[code423n4]

LOW 01: Missing documentation for bool roundUp in docstring

In AmountDeriver._applyFraction
All parameters are documented properly except for bool roundUp which does not have an explanation in the function documentation.

Recommendation

Add the parameter to the doc string;

 * @param roundUp     A boolean indicating whether the resultant amount
 *                    should be rounded up or down.

LOW 02 TokenTransferrer contract is missing contract documentation

The contact TokenTransferrer should have general contract documentation above the contract code, similar to all other contracts in the protocol, in such format:

/**
* @title TokenTransferrer
* @author
* @notice
*/

LOW 03 ERC712 transfers should use safeTransferFrom instead of transferFrom

Currently, the ERC721Interface contains only transferFrom. Hence, for many ERC721 tokens which have safeTransferFrom as well, the non safe function will get called. By openzeppeling ERC721 standard the safe transfer is safer:

 * @dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients
 * are aware of the ERC721 protocol to prevent tokens from being forever locked.

I suggest the ERC721Interface should use the safeTransferFrom, and then, for example, in TokenTransferrer._performERC721Transfer
the safer transfer method will get executed

LOW 04: Should not assume the last route in else statement

In ReferenceBasicOrderFulfiller._validateAndFulfillBasicOrder, there are many if-else statements which check the route. the last else assumes the routh is the last one left. This is problematic as the route can be not an expected value and any value will enter the else.
Instead, I recommend to use else if for all values, and finish with an else ... revert InvalidRoute().

For example:

 ItemType receivedItemType;
    if (
        route == BasicOrderRouteType.ETH_TO_ERC721 ||
        route == BasicOrderRouteType.ETH_TO_ERC1155
    ) {
        receivedItemType = ItemType.NATIVE;
    } else if (
        route == BasicOrderRouteType.ERC20_TO_ERC721 ||
        route == BasicOrderRouteType.ERC20_TO_ERC1155
    ) {
        receivedItemType = ItemType.ERC20;
    } else if (route == BasicOrderRouteType.ERC721_TO_ERC20) {
        receivedItemType = ItemType.ERC721;
    } else {
        receivedItemType = ItemType.ERC1155;
    }

should be

 ItemType receivedItemType;
    if (
        route == BasicOrderRouteType.ETH_TO_ERC721 ||
        route == BasicOrderRouteType.ETH_TO_ERC1155
    ) {
        receivedItemType = ItemType.NATIVE;
    } else if (
        route == BasicOrderRouteType.ERC20_TO_ERC721 ||
        route == BasicOrderRouteType.ERC20_TO_ERC1155
    ) {
        receivedItemType = ItemType.ERC20;
    } else if (route == BasicOrderRouteType.ERC721_TO_ERC20) {
        receivedItemType = ItemType.ERC721;
    } else if (routh == BasicOrderRouteType.ERC1155_TO_ERC20) {
        receivedItemType = ItemType.ERC1155;
    } else {
         revert ...
    }

[HardlyDifficult]

Missing documentation for bool roundUp in docstring
TokenTransferrer contract is missing contract documentation

Yes, ideally the documentation coverage would be complete - but these are very much nice to have improvements.

ERC712 transfers should use safeTransferFrom instead of transferFrom

Using safeTransferFrom is a common best practice to recommend. In this project it was intentionally avoided -- see the sponsor comment here #173 (comment) and #19 (comment)

Should not assume the last route in else statement

The sentiment here is valid, but the current code does cover all the scenarios. If they were to make the recommended change it would be more difficult to validate it with 100% code coverage unless a mock was used to push through inputs that otherwise would not occur.