search

code-423n4 / 2022-05-opensea-seaport-findings

1 stars 0 forks source link

QA Report #213

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

1. Accumulator armed check uses hardcoded value (low)

Now the implementation uses hard code in one place and AccumulatorDisarmed constant in another:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/Executor.sol#L433-L435

        if (accumulator.length != 64) {
            return;
        }

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/Executor.sol#L599-L601

        // "Arm" and prime accumulator if it's not already armed. The sentinel
        // value is held in the length of the accumulator array.
        if (accumulator.length == AccumulatorDisarmed) {

Also, in reference implementation the same accumulator.length != 64 check is inverted:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/reference/lib/ReferenceExecutor.sol#L313-L316

        // Exit if the accumulator is not "armed".
        if (accumulatorStruct.transfers.length == 0) {
            return;
        }

Recommended Mitigation Steps

Consider using the constant as it comes with no additional costs, reduces futher development risks and increases the code transparency:

        if (accumulator.length != AccumulatorArmed) {
            return;
        }

or use the same approach as in reference and in _insert, which is even more clear:

        if (accumulator.length == AccumulatorDisarmed) {
            return;
        }

It boils down to the desired behavior on a malfunction: if bogus value happen to reside at accumulator.length, should accumulator be processed so no transfers are omitted or should no operation take place?

It is also prudent to have a revert if the value found at length slot is neither AccumulatorDisarmed, nor AccumulatorArmed, so no transfers would happen if the structure is somehow corrupted. The additional cost of one check here will guard possible wrong sequence of external token transfers.

2. The list of order characteristics can be repeated exactly, yielding the same hash (low)

Several fully similar orders can be simultaneously placed by an offerer not knowing the specifics of the system.

Their characteristics can be exactly the same. For example, nothing prevents an offerer from placing 5 fully similar offers for a particular ERC1155. Formally this would be a correct usage of the system, but it will lead to the first offer being utilized, while all the others being stuck. I.e. it is a use case limitation and currently it should be enforced by UI that such cases will have one offer for 5 ERC155 with partial fulfillments enabled instead, otherwise some users will experience unexpected behavior that can even stretch to a losses (say if other 4 sales were urgently needed for an offerer).

Proof of Concept

Orders are being addressed by hash determined by the set of characteristics, that can be fully repeated in distinct orders:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderValidator.sol#L284-L300

    // Derive order hash using the order parameters and the nonce.
    bytes32 orderHash = _deriveOrderHash(
        OrderParameters(
            offerer,
            zone,
            order.offer,
            order.consideration,
            order.orderType,
            order.startTime,
            order.endTime,
            order.zoneHash,
            order.salt,
            order.conduitKey,
            order.consideration.length
        ),
        order.nonce
    );

Second order with all the same characteristics will not be fillable as it will have the same hash, while its numerator and denominator will stay, for example, to be equal to 1 if the first order was fulfilled with fulfillBasicOrder():

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderValidator.sol#L69-L74

        // Update order status as fully filled, packing struct values.
        _orderStatus[orderHash].isValidated = true;
        _orderStatus[orderHash].isCancelled = false;
        _orderStatus[orderHash].numerator = 1;
        _orderStatus[orderHash].denominator = 1;
    }

Recommended Mitigation Steps

Consider introducing userSalt to be used as an additional source of randomness, say to be purposely filled at random at UI level (say based on the interface interactions and so on) so even several similar orders placed into the same block will have different userSalt. I.e. salt being system-level seed, userSalt being user-level one based on user's actions.

3. _orderStatus is never cleaned up

_orderStatus will grow over time with system usage:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderValidator.sol#L26-L28

contract OrderValidator is Executor, ZoneInteraction {
    // Track status of each order (validated, cancelled, and fraction filled).
    mapping(bytes32 => OrderStatus) private _orderStatus;

Recommended Mitigation Steps

Consider introducing a function to clean up any expired entry (i.e. with endTime low enough) so the size of the mapping can be controlled

4. Comment diverging from the logic (low)

The _aggregateAvailable() concluding piece of logic sets the recipient to the offerer if no amount left so it be filtered out. Comment says otherwise:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/FulfillmentApplier.sol#L180-L183

            // Set the offerer as the receipient if execution amount is nonzero.
            if (execution.item.amount == 0) {
                execution.item.recipient = payable(execution.offerer);
            }

Recommended Mitigation Steps

-           // Set the offerer as the receipient if execution amount is nonzero.
+           // Set the offerer as the receipient if execution amount is zero.
            if (execution.item.amount == 0) {
                execution.item.recipient = payable(execution.offerer);
            }

5. Leftover comment from another _transfer function (low)

Conduit's _transfer doesn't deal with native tokens, but have a possibly leftover comment from Executor's _transfer:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/Conduit.sol#L174-L176

    function _transfer(ConduitTransfer calldata item) internal {
        // If the item type indicates Ether or a native token...
        if (item.itemType == ConduitItemType.ERC20) {

Recommended Mitigation Steps

Consider either removing the comment or expanding it detailing the absence of the native token case in Conduit._transfer.

6. Comment is misleading (low/non-critical)

Proof of Concept

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/lib/ConduitEnums.sol#L4-L5

enum ConduitItemType {
    NATIVE, // unused

ItemType.NATIVE is used across the system:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/BasicOrderFulfiller.sol#L173

if (additionalRecipientsItemType == ItemType.NATIVE) {

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/Executor.sol#L61

if (item.itemType == ItemType.NATIVE) {

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderCombiner.sol#L627

if (item.itemType == ItemType.NATIVE) {

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderFulfiller.sol#L245

if (offerItem.itemType == ItemType.NATIVE) {

Recommended Mitigation Steps

Consider expanding to explain more narrow use of NATIVE vs other item types.

7. Floating pragma is used in several contracts across the system (non-critical)

As different compiler versions have substantial specifics if the contracts get accidentally deployed using another compiler version compared to the one they were tested with, various types of undesired behavior can be introduced.

Proof of Concept

Unlike most system's contracts, those do not have compiler version fixed:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/Conduit.sol#L2

pragma solidity >=0.8.7;

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/TokenTransferrer.sol#L2

pragma solidity >=0.8.7;

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/TokenTransferrerConstants.sol#L2

pragma solidity >=0.8.7;

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/ConsiderationConstants.sol#L2

pragma solidity >=0.8.7;

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/ConsiderationEnums.sol#L2

pragma solidity >=0.8.7;

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/ConsiderationStructs.sol#L2

pragma solidity >=0.8.7;

Recommended Mitigation Steps

Consider fixing the version to 0.8.13 for the whole set of system contracts.

8. ReferenceExecutor uses hard coded itemTypes (non-critical)

Using hard coded values can lead to operational mistakes on further expansion of the system as it is reasonably hard to keep track of the correspondences.

Proof of Concept

ReferenceExecutor uses hard coded values for item types:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/reference/lib/ReferenceExecutor.sol#L165

uint256(1),

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/reference/lib/ReferenceExecutor.sol#L218

uint256(2),

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/reference/lib/ReferenceExecutor.sol#L269

uint256(3),

Recommended Mitigation Steps

Consider using the easily readable itemType constants from contracts/lib/ConsiderationEnums.sol

HardlyDifficult commented 2 years ago

Accumulator armed check uses hardcoded value

It is nice to be more consistent in the code. There does not appear to be any real bug here though.

The list of order characteristics can be repeated exactly, yielding the same hash

It is possible some confusing behavior could result here. But isn't this one of the reasons order.salt is included? Maybe I'm overlooking something here but it seems adding userSalt could lead to the same issue and would not really change the equation here.

_orderStatus is never cleaned up

Clean up could be done but it adds complexity without a clear benefit to the protocol. If rent were introduced then this may be something to reconsider.

Comment diverging from the logic Leftover comment from another _transfer function Comment is misleading

Agree the comments should be fixed to avoid confusion.

Floating pragma is used in several contracts across the system

The sponsor commented that this was necessary for reference contracts https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/85#issuecomment-1145577572 -- in the end their config selects which compiler version is to be used, they deployed 1.0 with 0.8.13 and 1.1 with 0.8.14.

ReferenceExecutor uses hard coded itemTypes

Agree using constants would improve readability here.