OpenSea Seaport 1.2 contest - RaymondFam's results

Interfaces and libraries should be separately saved and imported

Some contracts have interface(s) or libraries showing up in its/their entirety at the top/bottom of the contract facilitating an ease of references on the same file page. This has, in certain instances, made the already large contract size to house an excessive code base. Additionally, it might create difficulty locating them when attempting to cross reference the specific interfaces embedded elsewhere but not saved into a particular .sol file.

Consider saving the interfaces and libraries entailed respectively, and having them imported like all other files.

Here are some of the instances entailed:

File: PointerLibraries.sol

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Some of the code bases, particularly those entailing assembly, are lacking partial/full NatSpec that will be of added values to the users and developers if adequately provided.

Here are some of the instances entailed:

File: PointerLibraries.sol File: ConduitStructs.sol File: ConduitEnums.sol

Typo/grammatical mistakes

File: ConsiderationDecoder.sol#L41

-            // Derive the size of the bytes array, rounding up to nearest word
+            // Derive the size of the bytes array, rounding up to the nearest word

File: OrderValidator.sol#L370

-     * @dev Internal pure function to check the compatibility of two offer
+     * @dev Internal pure function to check the compatibility of two offers

File: OrderValidator.sol#L38

- *         and updating their status.
+ *         and updating their statuses.

File: ConsiderationDecoder.sol#L875-L876

-                        // Set first word of scratch space to 0 so length of
+                        // Set first word of scratch space to 0 so lengths of
                        // offer/consideration are set to 0 on invalid encoding.

constant variables should be capitalized

Constants should be named with all capital letters with underscores separating words if need be.

For instance, in ConsiderationConstants.sol, all other constants should conform to the full capitalized standard adopted on lines 52 - 54:

File: ConsiderationConstants.sol

            [ ...]

- uint256 constant information_versionWithLength = 0x03312e32; // 1.2
+ uint256 constant INFORMATION_VERSIONWITHLENGTH = 0x03312e32; // 1.2
- uint256 constant information_length = 0xa0;
+ uint256 constant INFORMATION_LENGTH = 0xa0;

uint256 constant _NOT_ENTERED = 1;
uint256 constant _ENTERED = 2;
uint256 constant _ENTERED_AND_ACCEPTING_NATIVE_TOKENS = 3;

- uint256 constant Offset_fulfillAdvancedOrder_criteriaResolvers = 0x20;
+ uint256 constant OFFSET_FULFILLADVANCEDORDER_CRITERIARESOLVERS = 0x20;

            [ ... ]

Use delete to clear variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address or a boolean to false. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic.

For instance, the a = false instance below may be refactored as follows:

File: OrderValidator.sol#L88

-        orderStatus.isCancelled = false;
+        delete orderStatus.isCancelled;

Sanity checks at the constructor

Adequate zero address and zero value checks should be implemented at the constructor to avoid accidental error(s) that could result in non-functional calls associated with it particularly when assigning immutable variables.

For instance, the specific instance below may be refactored as follows:

File: OrderValidator.sol#L55

-    constructor(address conduitController) Executor(conduitController) {}
+    constructor(address conduitController) Executor(conduitController) {
+            require(conduitController != address(0));
+    }

abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked(), it can often be cast to bytes() or bytes32() instead. If all arguments are strings and or bytes, bytes.concat() should be used instead.

For instance, the specific instance below may be refactored as follows:

File: ConduitController.sol#L72-L85

        conduit = address(
            uint160(
                uint256(
                    keccak256(
-                        abi.encodePacked(
+                        abi.encode(
                            bytes1(0xff),
                            address(this),
                            conduitKey,
                            _CONDUIT_CREATION_CODE_HASH
                        )
                    )
                )
            )
        );

Solidity's Style Guide on contract layout

According to Solidity's Style Guide below:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:

constructor, receive function (if exists), fallback function (if exists), external, public, internal, private

And, within a grouping, place the view and pure functions last.

Additionally, inside each contract, library or interface, use the following order:

type declarations, state variables, events, modifiers, functions

Consider adhering to the above guidelines for all contract instances entailed.

Descriptive and verbose options

Consider making the naming of local variables more verbose and descriptive so all other peer developers would better be able to comprehend the intended statement logic, significantly enhancing the code readability.

For instance, the local variables of the function below may be rewritten as follows:

File: PointerLibraries.sol#L48-L55

    function lt(
-        CalldataPointer a,
+        CalldataPointer aCdPtr,
-        CalldataPointer b
+        CalldataPointer bCdPtr
-    ) internal pure returns (bool c) {
+    ) internal pure returns (bool status_) {
        assembly {
-            c := lt(a, b)
+            status_ := lt(aCdPtr, bCdPtr)
        }
    }

Modularity on import usages

For cleaner Solidity code in conjunction with the rule of modularity and modular programming, use named imports with curly braces instead of adopting the global import approach.

For instance, the import instances below could be refactored conforming to most other instances in the code bases as follows:

File: ConsiderationDecoder.sol#L20-L22

- import "./ConsiderationConstants.sol";
+ import {ConsiderationConstants} from "./ConsiderationConstants.sol";

- import "../helpers/PointerLibraries.sol";
+ import {PointerLibraries} from "../helpers/PointerLibraries.sol";

Function Calls in Loop Could Lead to Denial of Service

Function calls made in unbounded loop are error-prone with potential resource exhaustion as it can trap the contract due to the gas limitations or failed transactions.

For instance, the iteration in the for loop below over each pointer, word by word, until the tail is reached could end up running out of gas if a huge array devoid of a boundary option is entailed:

File: ConsiderationDecoder.sol#L399-L405

            for (uint256 offset = 0; offset < tailOffset; offset += OneWord) {
                // Resolve Order calldata offset, use it to decode and copy from
                // calldata, and write resultant AdvancedOrder offset to memory.
                mPtrHead.offset(offset).write(
                    _decodeOrderAsAdvancedOrder(cdPtrHead.pptr(offset))
                );
            }

Use a more recent version of solidity

The protocol adopts version 0.8.7 on quite a number of contracts. For better security, it is best practice to use the latest Solidity version, 0.8.17.

Security fix list in the versions can be found in the link below:

https://github.com/ethereum/solidity/blob/develop/Changelog.md

Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier instance below may be refactored as follows:

File: Conduit.sol#L40-L68

    function _onlyOpenChannel() private view {
        // Utilize assembly to access channel storage mapping directly.
        assembly {
            // Write the caller to scratch space.
            mstore(ChannelKey_channel_ptr, caller())

            // Write the storage slot for _channels to scratch space.
            mstore(ChannelKey_slot_ptr, _channels.slot)

            // Derive the position in storage of _channels[msg.sender]
            // and check if the stored value is zero.
            if iszero(
                sload(keccak256(ChannelKey_channel_ptr, ChannelKey_length))
            ) {
                // The caller is not an open channel; revert with
                // ChannelClosed(caller). First, set error signature in memory.
                mstore(ChannelClosed_error_ptr, ChannelClosed_error_signature)

                // Next, set the caller as the argument.
                mstore(ChannelClosed_channel_ptr, caller())

                // Finally, revert, returning full custom error with argument.
                revert(ChannelClosed_error_ptr, ChannelClosed_error_length)
            }
        }
    }

    modifier onlyOpenChannel() {
        _onlyOpenChannel();
        _;
    }

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.13",

settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Payable access control functions costs less gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

For instance, the function below may be refactored as follows:

File: Conduit.sol#L92-L111

    function execute(
        ConduitTransfer[] calldata transfers
-    ) external override onlyOpenChannel returns (bytes4 magicValue) {
+    ) external payable override onlyOpenChannel returns (bytes4 magicValue) {

            [ ... ]

+= and -= cost more gas

+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.

For instance, the += instance below may be refactored as follows:

File: ConsiderationDecoder.sol#L673

-            for (uint256 offset = 0; offset < tailOffset; offset += OneWord) {
+            for (uint256 offset = 0; offset < tailOffset; offset = offset + OneWord) {

Similarly, the -= instance below may be refactored as follows:

File: BasicOrderFulfiller.sol#L965

-                nativeTokensRemaining -= additionalRecipientAmount;
+                nativeTokensRemaining = nativeTokensRemaining - additionalRecipientAmount;

Use of named returns for local variables saves gas

You can have further advantages in terms of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: BasicOrderFulfiller.sol#L68-L276

    function _validateAndFulfillBasicOrder(
        BasicOrderParameters calldata parameters
-    ) internal returns (bool) {
+    ) internal returns (bool status_) {

    [ ... ]

-        return true;
+        status_ = true;
    }

Use storage instead of memory for structs/arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

For instance, the specific instance below may be refactored as follows:

File: OrderValidator.sol#L118

-        OrderParameters memory orderParameters = advancedOrder.parameters;
+        OrderParameters storage orderParameters = advancedOrder.parameters;

++i costs less gas than i++, especially when it's used in for loops (--i / i-- too)

The post-increment (i++ or i--) operation costs more (about 5 GAS per iteration), because the compiler typically has to create a temporary variable for returning the initial value of i when incrementing/decrementing i . In contrast, the pre-increment (++i or --i) operation simply returns the actual incremented value of i without the need for a temporary variable. The saving impact is particularly prominent when dealing with loops.

For instance, the i-- instance below may be refactored as follows:

File: OrderCombiner.sol#L272

-                maximumFulfilled--;
+                --maximumFulfilled;

Ternary over if ... else

Using ternary operator instead of the if else statement saves gas.

For instance, the specific instance below may be refactored as follows:

File: OrderCombiner.sol#L322-L332

 offerItem.startAmount == offerItem.endAmount
     ? offerItem.startAmount = endAmount
     : offerItem.startAmount = _getFraction(
                            numerator,
                            denominator,
                            offerItem.startAmount
                        );

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, consider replacing >= with the strict counterpart > in the following inequality instance:

File: ConduitController.sol#L432

-        if (channelIndex >= totalChannels) {
// Rationale for subtracting 1 on the right side of the inequality:
// x >= 10, [10, 11, 12, ...]
// x > 10 - 1 is the same as x > 9, [10, 11, 12 ...] 
+        if (channelIndex > totalChannels - 1) {

|| costs less gas than its equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

As an example, the instance below may be refactored as follows:

File: ConduitController.sol#L147

-        if (isOpen && !channelPreviouslyOpen) {
+        if (!(!isOpen || channelPreviouslyOpen)) {