OpenSea Seaport contest - Spearbit's results

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderValidator.sol#L228 https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderValidator.sol#L231 https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderValidator.sol#L237 https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderValidator.sol#L238

Vulnerability details

Impact

A partial order's fractions (numerator and denominator) can be reset to 0 due to a truncation. This can be used to craft malicious orders:

1. Consider user Alice, who has 100 ERC1155 tokens, who approved all of their tokens to the marketplaceContract.
2. Alice places a PARTIAL_OPEN order with 10 ERC1155 tokens and consideration of ETH.
3. Malory tries to fill the order in the following way:
   1. Malory tries to fill 50% of the order, but instead of providing the fraction 1 / 2, Bob provides 2**118 / 2**119. This sets the totalFilled to 2**118 and totalSize to 2**119.
   2. Malory tries to fill 10% of the order, by providing 1 / 10. The computation 2**118 / 2**119 + 1 / 10 is done by "cross multiplying" the denominators, leading to the acutal fraction being numerator = (2**118 * 10 + 2**119) and denominator = 2**119 * 10.
   3. Because of the uint120 truncation in OrderValidator.sol#L228-L248, the numerator and denominator are truncated to 0 and 0 respectively.
   4. Bob can now continue filling the order and draining any approved (1000 tokens in total) of the above ERC1155 tokens, for the same consideration amount!

Proof of Concept

For a full POC: https://gist.github.com/hrkrshnn/7c51b23f7c43c55ba0f8157c3b298409

The following change would make the above POC fail:

modified   contracts/lib/OrderValidator.sol
@@ -225,6 +225,8 @@ contract OrderValidator is Executor, ZoneInteraction {
                 // Update order status and fill amount, packing struct values.
                 _orderStatus[orderHash].isValidated = true;
                 _orderStatus[orderHash].isCancelled = false;
+                require(filledNumerator + numerator <= type(uint120).max, "overflow");
+                require(denominator <= type(uint120).max, "overflow");
                 _orderStatus[orderHash].numerator = uint120(
                     filledNumerator + numerator
                 );
@@ -234,6 +236,8 @@ contract OrderValidator is Executor, ZoneInteraction {
             // Update order status and fill amount, packing struct values.
             _orderStatus[orderHash].isValidated = true;
             _orderStatus[orderHash].isCancelled = false;
+            require(numerator <= type(uint120).max, "overflow");
+            require(denominator <= type(uint120).max, "overflow");
             _orderStatus[orderHash].numerator = uint120(numerator);
             _orderStatus[orderHash].denominator = uint120(denominator);
         }

Tools Used

Manual review

Recommended Mitigation Steps

A basic fix for this would involve adding the above checks for overflow / truncation and reverting in that case. However, we think the mechanism is still flawed in some respects and require more changes to fully fix it. See a related issue: "A malicious filler can fill a partial order in such a way that the rest cannot be filled by anyone" that points out a related but a more fundamental issue with the mechanism.

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/FulfillmentApplier.sol#L406

Vulnerability details

Impact

The _aggregateValidFulfillmentOfferItems() function aims to revert on orders with zero value or where a total consideration amount overflows. Internally this is accomplished by having a temporary variable errorBuffer, accumulating issues found, and only reverting once all the items are processed in case there was a problem found. This code is optimistic for valid inputs.

Note: there is a similar issue in _aggregateValidFulfillmentConsiderationItems() , which is reported separately.

The problem lies in how this errorBuffer is updated:

                // Update error buffer (1 = zero amount, 2 = overflow).
                errorBuffer := or(
                  errorBuffer,
                  or(
                    shl(1, lt(newAmount, amount)),
                    iszero(mload(amountPtr))
                  )
                )

The final error handling code:

            // Determine if an error code is contained in the error buffer.
            switch errorBuffer
            case 1 {
                // Store the MissingItemAmount error signature.
                mstore(0, MissingItemAmount_error_signature)

                // Return, supplying MissingItemAmount signature.
                revert(0, MissingItemAmount_error_len)
            }
            case 2 {
                // If the sum overflowed, panic.
                throwOverflow()
            }

While the expected value is 0 (success), 1 or 2 (failure), it is possible to set it to 3, which is unhandled and considered as a "success". This can be easily accomplished by having both an overflowing item and a zero item in the order list.

This validation error could lead to fulfilling an order with a consideration (potentially ~0) lower than expected.

Proof of Concept

Craft an offer containing two errors (e.g. with zero amount and overflow). Call matchOrders(). Via calls to _matchAdvancedOrders(), _fulfillAdvancedOrders(), _applyFulfillment(), _aggregateValidFulfillmentOfferItems() will be called. The errorBuffer will get a value of 3 (the or of 1 and 2). As the value of 3 is not detected, no error will be thrown and the order will be executed, including the mal formed values.

Tools Used

Manual review

Recommended Mitigation Steps

1. Change the check on FulfillmentApplier.sol#L465 to consider case 3.
2. Potential option: Introduce an early abort in case errorBuffer != 0 on FulfillmentApplier.sol#L338

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/CriteriaResolution.sol#L155 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/CriteriaResolution.sol#L241

Vulnerability details

Impact

_verifyProof allows empty proofs and in that case it expects the leaf to equal the root, because no hashing and iteration is taking place. The purpose of the tree is to hold multiple accepted tokenIds, where the consideration contains one and proving its existence in the order via the proof. Because of this the leaf equals to a single tokenId.

This can manifest in distinct issues:

1. A criteria order with an empty proof is the same as a non-criteria order with the exception of the ItemType is differing (e.g. ItemType.ERC721 vs ItemType.ERC721_WITH_CRITERIA). This is a slight malleability symptom.

2. In the case of leaf=0 (tokenId=0) the _verifyProof function is not even executed.

Proof of Concept

Context: CriteriaResolution.sol#L241, CriteriaResolution.sol#L155

Tools Used

Manual review

Recommended Mitigation Steps

Hash the leaf node or disallow empty proofs.

We have an extensive QA report with 23 different findings in https://gist.github.com/hrkrshnn/d08e02519a172ba938195093adba0a09

Submitting it as a separate gist due to size limit, and also to have syntax highlighting and MathJax support.

Define function pointers in constructor

Impact

The function _applyFractionsAndTransferEach() contains two function pointer assignments, which are difficult to read. They could also be set in the constructor and stored immutable, which will make the code more readable and save some gas.

Proof of Concept

Context: OrderFulfiller.sol#L199-L214, OrderFulfiller.sol#L289-L303

contract OrderFulfiller is ...
    function _applyFractionsAndTransferEach(...) ... {
        ...
        function(OfferItem memory, address, bytes32, bytes memory) internal _transferOfferItem;
        function(ReceivedItem memory, address, bytes32, bytes memory) internal _transferReceivedItem = _transfer;
        assembly {  _transferOfferItem := _transferReceivedItem  }
        ...
        function(ConsiderationItem memory, address, bytes32, bytes memory) internal _transferConsiderationItem;
        function(ReceivedItem memory, address, bytes32, bytes memory) internal _transferReceivedItem = _transfer;
        assembly {   _transferConsiderationItem := _transferReceivedItem  }
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider setting the function pointers in the constructor, as shown in the following example:

//SPDX-License-Identifier: MIT
pragma solidity 0.8.14;
import "hardhat/console.sol"; 

contract TestFunctionPointers{
    struct OfferItem    { uint256 startAmount; }
    struct ReceivedItem { uint256 identifier;  }
    function _transfer(OfferItem memory oi) public {
        console.log("In _transfer",oi.startAmount);
    }
    function(ReceivedItem memory) internal immutable _transferReceivedItem;
    constructor() {
        function(OfferItem memory)    internal fpa = _transfer;
        function(ReceivedItem memory) internal fpb;
        assembly { fpb := fpa }
        _transferReceivedItem = fpb;
        ReceivedItem memory ri;
        ri.identifier = 1234567;
        _transferReceivedItem(ri);
    }  
}

Gas saving in _verifyProof()

Impact

The function _verifyProof() of contract CriteriaResolution uses a switch statement. This can be optimized to run withouth a jump to save some gas. As this is within a loop, it might be worth the effort.

Proof of Concept

Context: CriteriaResolution.sol#L265-L273

This is the current code:

function _verifyProof( ... ) ... {
    ...
    assembly {
        ...
        for { ... } { ... }
            ...
            switch gt(computedHash, loadedData)  // here is a jump
                case 0 {
                    mstore(0, computedHash) // Place existing hash first.
                    mstore(0x20, loadedData) // Place new hash next.
                }
                default {
                    mstore(0, loadedData) // Place new hash first.
                    mstore(0x20, computedHash) // Place existing hash next.
                }
           ...
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider using the following:

assembly {
    let g := mul( gt(computedHash, loadedData), 0x20)
    mstore(g, computedHash )
    mstore(sub(0x20,g), loadedData )
}

Here is some test code to see the difference:

//SPDX-License-Identifier: MIT
pragma solidity 0.8.14;
import "hardhat/console.sol"; 

contract TestStore{
    function min(uint a,uint b) internal pure returns(uint) {
        if (a<b) return a; else return b;
    }

    function t1(uint computedHash , uint loadedData) internal {
        assembly {
            switch gt(computedHash, loadedData)
                case 0 {
                    mstore(0, computedHash) 
                    mstore(0x20, loadedData) 
                }
                default {
                    mstore(0, loadedData) 
                    mstore(0x20, computedHash) 
                }
        }
    }

    function t2(uint computedHash , uint loadedData) internal {
        assembly {
            let g := mul( gt(computedHash, loadedData), 0x20)
            mstore(g, computedHash )
            mstore(sub(0x20,g), loadedData )
        }
    }

    constructor()  {
        uint n=11;uint g1;uint g2;uint g=type(uint).max;uint i;
        g=type(uint).max;g1=gasleft();for(i=0;i<n;i++) { t1(i,4); } g2=gasleft();g=min(g,g1-g2);console.log(g/n); // 284
        g=type(uint).max;g1=gasleft();for(i=0;i<n;i++) { t2(i,4); } g2=gasleft();g=min(g,g1-g2);console.log(g/n); // 268
    }
}

Gas saving by replacing the if condition with || by a branchless expression

Impact

  if (numerator > denominator || numerator == 0) {
    ...
  }

The condition can be replaced by or(iszero(numerator), gt(numerator, denominator)) which saves one jumpi.

Proof of Concept

Context: Seaport.sol#L137

Tools Used

Manual review

Recommended Mitigation Steps

Replace the if statement by the equivalent code in assembly (mentioned above).