OpenSea Seaport contest - ilan's results

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 ...
    }

 

G01 No need to explicitly initialize variables with default values

If a variable is not initialized it is automatically set to the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Example Instance: uint256 extraCeiling = 0; -> uint256 extraCeiling;

In AmountDeriver._locateCurrentAmount

This pattern is used everywhere in the protocol.

G02 In some places, loop memory variables which don't change are not cached

Calling storage/memory variable which does not change, every loop iteration, is wrong and wastes gas. In most places this is taken care of, however not everywhere.

For example, OrderCombiner.sol#L247:

// Retrieve array of offer items for the order in question.
OfferItem[] memory offer = advancedOrder.parameters.offer;

// Iterate over each offer item on the order.
for (uint256 j = 0; j < offer.length; ++j) {
    // Retrieve the offer item.
    OfferItem memory offerItem = offer[j];

instead, cache the length:

// Retrieve array of offer items for the order in question.
OfferItem[] memory offer = advancedOrder.parameters.offer;

uint256 totalOffers = offer.length
// Iterate over each offer item on the order.
for (uint256 j = 0; j < totalOffers; ++j) {
    // Retrieve the offer item.
    OfferItem memory offerItem = offer[j];

Other Instances Include (but not limited to):

OrderCombiner.sol#L291

OrderCombiner.sol#L621

OrderFulfiller.sol#L217

OrderFulfiller.sol#L306

ReferenceBasicOrderFulfiller.sol#L643

ReferenceBasicOrderFulfiller.sol#L712