OpenSea Seaport contest - hickuphh3's results

Codebase Impressions & Summary

My CS professor once said, code is like art. There are some pieces you come across that you can’t help but admire at their elegance and beauty. Seaport is one such piece of work that the optimizoooooorr gurus have blessed us with. Substantial portions of the codebase are written in assembly, with various gas optimization tricks sprinkled to ensure the code is as gas efficient as it can be.

The reference implementation is a great addition to be used to check against the optimized version. This practice should be adopted by future projects looking to have assembly code in their contracts.

Documentation wise, the inline comments were useful, especially those that explained the stack and memory positions of the variables. A video walkthrough would have greatly helped in getting people up to speed with the codebase, especially for one this complex. It would serve as an extremely useful educational material as well!

Some of the gas optimization tricks used aren’t immediately intuitive, so explaining those in a video walkthrough or via inline comments would have been appreciated. An example of this is the SignatureVerification contract, where a couple of checks were omitted.

Overall, Seaport sets the gold standard of how to write a contract with extensive assembly code. Providing sufficient supporting material to explain what its intended behaviour is paramount in ensuring the code is behaving as intended.

Low Severity Findings

L01: Incorrect name filling description

Line References

https://github.com/ProjectOpenSea/seaport/blob/d8d4e471cef34b43fd370e20762816641550e9aa/contracts/lib/ConsiderationConstants.sol#L40

Description

The name is 13 bytes long, so it fills bytes 64-76, not to 77. It would be great to further describe the neat optimization trick used to store both the length and name via a single mstore in the comments too! Under-appreciated sorcery going on here =)

Recommended Mitigation Steps

name fills bytes 64-77 → name fills bytes 64-76

L02: Incorrect byte length hash description

Line References

https://github.com/ProjectOpenSea/seaport/blob/d8d4e471cef34b43fd370e20762816641550e9aa/contracts/lib/GettersAndDerivers.sol#L354

Description

The hash region is 66 bytes, not 65:

• 2 bytes for the EIP_712_PREFIX
• 32 bytes for domain separator
• 32 bytes for the order hash

Recommended Mitigation Steps

// Hash the relevant region (65 bytes). → // Hash the relevant region (66 bytes).

L03: Outdated comment and reference implementation on remainder check

Line References

https://github.com/ProjectOpenSea/seaport/blob/d8d4e471cef34b43fd370e20762816641550e9aa/contracts/lib/AmountDeriver.sol#L116

https://github.com/ProjectOpenSea/seaport/blob/d8d4e471cef34b43fd370e20762816641550e9aa/reference/lib/ReferenceAmountDeriver.sol#L96-L100

Description

The zero remainder check has been performed in L105 due to a proposed gas optimization using mulmod. The comment // Divide and check for remainder. and reference implementation is therefore outdated.

Recommended Mitigation Steps

AmountDeriver.sol

// Perform the division without zero check. Note that denominator cannot be zero.
assembly {
  newValue := div(valueTimesNumerator, denominator)
}

ReferenceAmountDeriver.sol

// insert this code block after L88
bool exact = mulmod(newValue, numerator, denominator) == 0;
if (!exact) {
  revert InexactFraction();
}

Non-Critical Findings

NC01: Explain why validity range of s is not checked

Line References

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L154-L165

Description

The valid range for s: 0 < s < secp256k1n ÷ 2 + 1 check has been omitted from the SignatureVerification contract. It would be great to explain why this is the case (presumably as a gas optimization).

The impact of omitting the check is that “it nevertheless creates a UI inconvenience as an attacker can cause the transaction that gets confirmed in a block to have a different hash from the transaction that any user sends, interfering with user interfaces that use transaction hashes as tracking IDs”, which we assume is an acceptable tradeoff in favour of reduced gas usage.

Reference

https://ethereum.stackexchange.com/questions/55245/why-is-s-in-transaction-signature-limited-to-n-21

NC02: Explain why validity range of v is not checked in EIP-2098 signatures

Line References

https://github.com/ProjectOpenSea/seaport/blob/d8d4e471cef34b43fd370e20762816641550e9aa/contracts/lib/SignatureVerification.sol#L60-L61

Description

v is typically checked to be either 27 or 28 regardless of signature length, as seen in OZ’s ECDSA library.

It’s not immediately intuitive the validity range of v isn’t checked: it’s taken from the highest bit, which means it’s either a 0 or 1, then incremented by 27, thus ensuring that v is definitely valid. It would be great to add this explanation as an inline comment.

NC03: Incorrect hexadecimal values in ReferenceAssertions._assertValidBasicOrderParameterOffsets() comment description

Line References

https://github.com/ProjectOpenSea/seaport/blob/d8d4e471cef34b43fd370e20762816641550e9aa/reference/lib/ReferenceAssertions.sol#L108-L109

https://github.com/ProjectOpenSea/seaport/blob/d8d4e471cef34b43fd370e20762816641550e9aa/contracts/lib/Assertions.sol#L114-L115

Description

The correct values for the Additional recipients arr offset and Signature offset should be 0x240 and 0x260 + (recipients.length * 0x40) respectively.

Recommended Mitigation Steps

* 2. Additional recipients arr offset == 0x240
* 3. Signature offset == 0x260 + (recipients.length * 0x40)

G01: Change mathematical expression for memory allocation computation

Line References

https://github.com/ProjectOpenSea/seaport/blob/348e4d59ac8b47e0dbd0a44264a7cbb3e175af0b/contracts/lib/LowLevelHelpers.sol#L71-L74

https://github.com/ProjectOpenSea/seaport/blob/d8d4e471cef34b43fd370e20762816641550e9aa/contracts/lib/TokenTransferrer.sol#L283-L286

https://github.com/ProjectOpenSea/seaport/blob/d8d4e471cef34b43fd370e20762816641550e9aa/contracts/lib/TokenTransferrer.sol#L417-L420

Description

The calculation is of the form (P.S. I hope GH markdown shows this nicely with their recent meth expression support) $a^2 - b^2$, which can be simplified to $(a+b) * (a-b)$. This will save 2 gas because 1 MUL opcode is changed to a ADD opcode.

Recommended Mitigation Steps

mul(
	add(returnDataWords, msizeWords),
	sub(returnDataWords, msizeWords)
),

A further optimization can be made to save the result of sub(returnDataWords, msizeWords) because it is performed twice.

G02: Multiplication and division by powers of 2 can be changed to bit shifting

Description

The mul and div opcodes consume 5 gas while the shl and shr opcodes consume 3. Thus, any multiplication and division by powers of 2 can be switched to shl and shr respectively. Notably, OneWord = 0x20 = 32 is a power of 2, and there are many instances where multiplication and division is done with OneWord.

Admittedly, this will come at the cost of readability. For instance,

mul(itemIndex, OneWord)

becomes

// OneWordBits = log2(0x20) = 5
shl(OneWordBits, itemIndex)

which isn’t as intuitive and readable.

G03: Use postfix decrement

Line References

https://github.com/ProjectOpenSea/seaport/blob/d8d4e471cef34b43fd370e20762816641550e9aa/contracts/lib/OrderCombiner.sol#L229

Reference

https://github.com/byterocket/c4-common-issues/blob/main/0-Gas-Optimizations.md/#g012---use-prefix-increment-instead-of-postfix-increment-if-possible

Description

The difference between the prefix increment / decrement and postfix increment / decrement expressions lie in the return value of the expression. The prefix expression (++i / --i) returns the updated value after it's incremented. The postfix expression (i++ / --i) returns the original value.

It is generally cheaper to use the prefix expression whenever possible.

Since maximumFulfilled isn’t used subsequently, it would be good to use the prefix increment. Interestingly, attempts to do the same for totalFilteredExecutions yielded worse results, where it is cheapest to do totalFilteredExecutions += 1.

Recommended Mitigation Steps

--maximumFulfilled;