LooksRare Aggregator contest - 0x1f8b's results

• Low
  • 1. Mixing and Outdated compiler
  • 2. One token will be locked forever
  • 3. Lack of checks supportsInterface
• Non critical
  • 4. Use abstract for base contracts
  • 5. Avoid hardcoded values
  • 6. IsAtomic design
  • 7. Use the same argument order

Low

1. Mixing and Outdated compiler

The pragma version used are:

pragma solidity ^0.8.0;
pragma solidity ^0.8.14;
pragma solidity 0.8.17;
pragma solidity >=0.5.0;

Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:

2. One token will be locked forever

When recovering lost tokens, one is always left in the contract, which produces an underflow if there is no balance, and in the worst case, leaves a token always locked.

I decided to mark this issue as low because the amount of tokens, although this subtraction is not reported in any comment of the code, nor is it justified. But keep in mind that depending on which tokens, a fraction could be of high value.

contract TokenRescuer is OwnableTwoSteps, LowLevelETH, LowLevelERC20Transfer {
    error InsufficientAmount();

    /**
     * @notice Rescue the contract's trapped ETH
     * @dev Must be called by the current owner
     * @param to Send the contract's ETH balance to this address
     */
    function rescueETH(address to) external onlyOwner {
-       uint256 withdrawAmount = address(this).balance - 1;
        if (withdrawAmount == 0) revert InsufficientAmount();
        _transferETH(to, withdrawAmount);
    }

    /**
     * @notice Rescue any of the contract's trapped ERC20 tokens
     * @dev Must be called by the current owner
     * @param currency The address of the ERC20 token to rescue from the contract
     * @param to Send the contract's specified ERC20 token balance to this address
     */
    function rescueERC20(address currency, address to) external onlyOwner {
-       uint256 withdrawAmount = IERC20(currency).balanceOf(address(this)) - 1;
        if (withdrawAmount == 0) revert InsufficientAmount();
        _executeERC20DirectTransfer(currency, to, withdrawAmount);
    }
}

Affected source code:

• TokenRescuer.sol:23
• TokenRescuer.sol:35

3. Lack of checks supportsInterface

The EIP-165 standard helps detect that a smart contract implements the expected logic, prevents human error when configuring smart contract bindings, so it is recommended to check that the received argument is a contract and supports the expected interface.

Reference:

• https://eips.ethereum.org/EIPS/eip-165

Affected source code:

• ERC20EnabledLooksRareAggregator.sol:22
• LooksRareProxy.sol:38-39
• SeaportProxy.sol:46-47

---

Non critical

4. Use abstract for base contracts

abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

The following contracts would be more convenient to be abstract:

• TokenRescuer.sol:14

The following contracts would be more convenient to implement as libraries:

• LowLevelERC1155Transfer.sol:11
• LowLevelERC20Approve.sol:11
• LowLevelERC20Transfer.sol:13
• LowLevelERC721Transfer.sol:11
• LowLevelETH.sol:11
• TokenTransferrer.sol:13

5. Avoid hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these values from the source code.

Affected source code:

Use the selector instead of the raw value:

• 0x1626ba7e > SignatureChecker.sol:80

Use a constant for 10000 in:

• SeaportProxy.sol:147
• SeaportProxy.sol:207
• LooksRareAggregator.sol:158

6. IsAtomic design

I don't see any advantage of having a global isAtomic argument to all commands, when this task can be done by the caller, it would make more sense to be able to independently execute one of all commands given in isAtomic way, so that argument should remain to the MakerOrder structure.

    function execute(
        BasicOrder[] calldata orders,
        bytes[] calldata ordersExtraData,
        bytes memory,
        address recipient,
-       bool isAtomic,
        uint256,
        address
    ) external payable override {

Affected source code:

• LooksRareProxy.sol:55
• SeaportProxy.sol:65
• LooksRareAggregator.sol:56

7. Use the same argument order

In LooksRareAggregator.approve, the argument order is different than in _executeERC20Approve, so it is prone to human error.

    function approve(
-       address marketplace,
        address currency,
+       address marketplace,
        uint256 amount
    ) external onlyOwner {
        _executeERC20Approve(currency, marketplace, amount);
    }

Affected source code:

• LooksRareAggregator.sol:171-177

• Gas
  • 1. There's no need to set default values for variables
    • Total gas saved: 8 * 1 = 8
  • 2. Reorder structure layout
  • 3. Use calldata instead of memory
  • 4. Avoid input decoding
  • 5. Reuse keys

Gas

1. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint256 a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint256 a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384

Affected source code:

• LooksRareProxy.sol:92

Total gas saved: 8 * 1 = 8

2. Reorder structure layout

The following structures could be optimized moving the position of certain values in order to save a lot slots:

• OrderComponents in ConsiderationStructs.sol:22-34:

struct OrderComponents {
    address offerer;
    address zone;
+   OrderType orderType;
    OfferItem[] offer;
    ConsiderationItem[] consideration;
-   OrderType orderType;
    uint256 startTime;
    uint256 endTime;
    bytes32 zoneHash;
    uint256 salt;
    bytes32 conduitKey;
    uint256 counter;
}

• BasicOrderParameters in ConsiderationStructs.sol:100-121

struct BasicOrderParameters {
    // calldata offset
    address considerationToken; // 0x24
    uint256 considerationIdentifier; // 0x44
    uint256 considerationAmount; // 0x64
    address payable offerer; // 0x84
    address zone; // 0xa4
    address offerToken; // 0xc4
+   BasicOrderType basicOrderType; // 0x124
    uint256 offerIdentifier; // 0xe4
    uint256 offerAmount; // 0x104
-   BasicOrderType basicOrderType; // 0x124
    uint256 startTime; // 0x144
    uint256 endTime; // 0x164
    bytes32 zoneHash; // 0x184
    uint256 salt; // 0x1a4
    bytes32 offererConduitKey; // 0x1c4
    bytes32 fulfillerConduitKey; // 0x1e4
    uint256 totalOriginalAdditionalRecipients; // 0x204
    AdditionalRecipient[] additionalRecipients; // 0x224
    bytes signature; // 0x244
    // Total length, excluding dynamic array data: 0x264 (580)
}

OrderParameters in ConsiderationStructs.sol:139-152

struct OrderParameters {
    address offerer; // 0x00
    address zone; // 0x20
+   OrderType orderType; // 0x80
    OfferItem[] offer; // 0x40
    ConsiderationItem[] consideration; // 0x60
-   OrderType orderType; // 0x80
    uint256 startTime; // 0xa0
    uint256 endTime; // 0xc0
    bytes32 zoneHash; // 0xe0
    uint256 salt; // 0x100
    bytes32 conduitKey; // 0x120
    uint256 totalOriginalConsiderationItems; // 0x140
    // offer.length                          // 0x160
}

Reference:

• https://ethdebug.github.io/solidity-data-representation

Enums are represented by integers; the possibility listed first by 0, the next by 1, and so forth. An enum type just acts like uintN, where N is the smallest legal value large enough to accomodate all the possibilities.

• https://docs.soliditylang.org/en/v0.8.17/internals/layout_in_storage.html#storage-inplace-encoding

3. Use calldata instead of memory

Some methods are declared as external but the arguments are defined as memory instead of as calldata.

By marking the function as external it is possible to use calldata in the arguments shown below and save significant gas.

Recommended changes:

• For TokenReceiver.sol:4-34:

abstract contract TokenReceiver {
    function onERC721Received(
        address,
        address,
        uint256,
        bytes calldata
    ) external pure returns (bytes4) {
        return this.onERC721Received.selector;
    }

    function onERC1155Received(
        address,
        address,
        uint256,
        uint256,
-       bytes memory
+       bytes calldata
    ) external virtual returns (bytes4) {
        return this.onERC1155Received.selector;
    }

    function onERC1155BatchReceived(
        address,
        address,
-       uint256[] memory,
-       uint256[] memory,
-       bytes memory
+       uint256[] calldata,
+       uint256[] calldata,
+       bytes calldata
    ) external virtual returns (bytes4) {
        return this.onERC1155BatchReceived.selector;
    }
}

• For LooksRareProxy.sol:53:

    function execute(
        BasicOrder[] calldata orders,
        bytes[] calldata ordersExtraData,
-       bytes memory,
+       bytes calldata,
        address recipient,
        bool isAtomic,
        uint256,
        address
    ) external payable override {

4. Avoid input decoding

It's possible to avoid decoding logic from the ordersExtraData in LooksRareProxy.execute.

    function execute(
        BasicOrder[] calldata orders,
-       bytes[] calldata ordersExtraData,
+       OrderExtraData[] calldata ordersExtraData,
        bytes memory,
        address recipient,
        bool isAtomic,
        uint256,
        address
    ) external payable override {
        if (address(this) != aggregator) revert InvalidCaller();

        uint256 ordersLength = orders.length;
        if (ordersLength == 0 || ordersLength != ordersExtraData.length) revert InvalidOrderLength();

        for (uint256 i; i < ordersLength; ) {
            BasicOrder memory order = orders[i];

-           OrderExtraData memory orderExtraData = abi.decode(ordersExtraData[i], (OrderExtraData));
+           OrderExtraData memory orderExtraData = ordersExtraData[i];

Affected source code:

• LooksRareProxy.sol:50-68
• SeaportProxy.sol:98
• SeaportProxy.sol:103

5. Reuse keys

It's possible to optimize the storage use of LooksRareAggregator joining the two mapping related to the proxy use to use only one with an struct.

    mapping(address => mapping(bytes4 => bool)) private _proxyFunctionSelectors;
    mapping(address => FeeData) private _proxyFeeData;

Could be changed to:

    struct Proxy {
        mapping(bytes4 => bool) functionSelectors;
        FeeData feeData;
    }

    mapping(address => Proxy) private _proxy;

Affected source code:

• LooksRareAggregator.sol:45-46