Seaport 1.1 #492

0age · 2022-06-10T07:34:59Z

This PR highlights changes from Seaport 1.0 to Seaport 1.1 and is meant to facilitate review and discussion of those changes.

Update CONTRIBUTORS.md

Revert on dirty bits (unused item parameters)

gas: add unchecked for ConduitController#updateChannel

Added ENS name of JasperAlexander

Assign basicOrderType to a variable rather than loading twice

Gas opt: skip `remaining` calculation when not necessary

implement nits from readthrough

Ensure revert on non-zero token / identifier (native) and identifier (erc20) in transferred item params

Add Foundry test for failures when spending pre-approved items from null address with invalid signature

…-reverts Test that "fulfill" methods revert on Orders with Native offer items

…-pass run one final comment pass over the token transferrer

include 1.1 contributors list in Seaport natspec

axic · 2022-06-11T13:31:46Z

contracts/conduit/Conduit.sol

        if (item.itemType == ConduitItemType.ERC20) {
-            // Transfer ERC20 token.
+            // Transfer ERC20 token. Note that item.identifier is ignored and


Is there a particular reason for this?

Avoiding a redundant check mainly, basically letting channels make the call as to whether they want to do this safety check or not

Avoiding a redundant check mainly,

Where is this checked before hand? The conduit can be used independently, can't it be?

axic · 2022-06-11T13:40:20Z

contracts/conduit/ConduitController.sol

@@ -198,8 +213,13 @@ contract ConduitController is ConduitControllerInterface {
            revert NewPotentialOwnerIsZeroAddress(conduit);
        }

+        // Ensure the new potential owner is not already set.
+        if (newPotentialOwner == _conduits[conduit].potentialOwner) {


This only checks for the same owner being proposed twice, but allows proposing a new one while a proposal is active. Is that the goal?

I think that’s fine, the current owner is still in control at that stage

It does feel like a not very useful check though.

axic · 2022-06-11T13:42:31Z

contracts/helpers/TransferHelper.sol

+                    if (item.itemType == ConduitItemType.NATIVE) {
+                        revert InvalidItemType();
+                    } else if (item.itemType == ConduitItemType.ERC20) {
+                        _performERC20Transfer(


This allows item.identifier to be anything.

axic · 2022-06-11T13:57:49Z

contracts/lib/OrderCombiner.sol

+            invalidNativeOfferItemErrorBuffer := shl(
+                1,
+                gt(
+                    // Take the remainder of the selector modulo a magic value.


👀

Which (two) selectors is this comparing? This seems like an extremely fragile piece of code should new external code paths allowed to reach this function.

For the reader: the shr is reading the selector (top 32 bits) and MagicModulus = 69, MagicRemainder = 0x1d.

It seems to be reachable from Consideration.matchOrders, Consideration.matchAdvancedOrders, Consideration.fulfillAvailableOrders, Consideration.fulfillAvailableAdvancedOrders.

That is 4 selectors this code is comparing. Note that not only new external functions are risky here, but also any parameter change to the above would change the selectors.

Found a prior conversation on the topic: https://github.com/ProjectOpenSea/seaport/pull/464/files#r893417290

My recommendation would be to pass down a isMatchOrder flag.

Yeah we’ll definitely replace this pattern with one like you suggest should any aspect of the interface change

axic · 2022-06-11T14:26:20Z

contracts/lib/OrderCombiner.sol

+        assembly {
+            // Use the second bit of the error buffer to indicate whether the
+            // current function is not matchAdvancedOrders or matchOrders.
+            invalidNativeOfferItemErrorBuffer := shl(


Likely no need for assembly on this and it clearly shows how risky it is to the reader:

unchecked { invalidNativeOfferItemErrorBuffer = ((uint32(msg.selector) % NonMatchSelector_MagicModulus) > NonMatchSelector_MagicRemainder) ? 2 : 0; }

Ok, for this need to have the optimiser support branchless ternary ops.

I do like including this as part of the documentation to demonstrate what’s happening here!

update deployment addresses

Thro77le · 2022-06-14T11:13:27Z

contracts/lib/ConsiderationBase.sol

@@ -184,7 +193,7 @@ contract ConsiderationBase is ConsiderationEventsAndErrors {
                "bytes32 zoneHash,",
                "uint256 salt,",
                "bytes32 conduitKey,",
-                "uint256 nonce",
+                "uint256 counter",


Documentation still contains old name (nonce instead of coutner)

naps62 and others added 30 commits June 8, 2022 15:47

code review

da9c12f

Merge branch 'main' into 0xleastwood-update-contributors

c60d5b6

Merge pull request #452 from 0xleastwood/0xleastwood-update-contributors

256c420

Update CONTRIBUTORS.md

Fix to reference Executor itemType

2203c31

Merge pull request #457 from axic/unused-params

5eb8906

Revert on dirty bits (unused item parameters)

extend original work from @axic

e096384

solve for stack issues with no viaIR

28e9b91

basic test passes

17796eb

Add a period

a834fb3

tests pass

5f71832

update comment

5686211

clean up

d4f9c5b

make ERC20 transfer on basic fulfillment agnostic

55d0325

add scuffed length check

941bd53

update basic test

63461ca

refactoring

d12f944

Added ENS name of JasperAlexander

4c6dd5b

Merge pull request #440 from mingwatch/mingwatch-gas-optimization-1

43467e5

gas: add unchecked for ConduitController#updateChannel

clean up comments and formatting

e8881cf

Merge pull request #462 from JasperAlexander/main

fbd7b0d

Added ENS name of JasperAlexander

Merge pull request #453 from ProjectOpenSea/basic-order-type-readability

15cb65a

Assign basicOrderType to a variable rather than loading twice

run linter

aa4060b

clean up imports

7d2b899

remove unused code

cc64d46

cleanup

c7d391f

add pragma to ConduitConstants

15bd8a0

add test overflow denominator to 0

5164ddd

Merge branch 'main' into main

fc93dcb

remove unused code

38be6fc

Merge pull request #448 from naps62/optimize-apply-fraction

084c9d0

Gas opt: skip `remaining` calculation when not necessary

0age and others added 14 commits June 10, 2022 17:31

Merge pull request #503 from ProjectOpenSea/readthrough-nits

3a23794

implement nits from readthrough

progress on final test

caa1d0d

close the loop finally

ad7cc31

Merge pull request #485 from ProjectOpenSea/unused-parameters-tests

1a65f0e

Ensure revert on non-zero token / identifier (native) and identifier (erc20) in transferred item params

test fulfills revert with native offer items

2b284d5

rename test for clarity

250dc70

Merge pull request #502 from ProjectOpenSea/test-null-spend

06bf918

Add Foundry test for failures when spending pre-approved items from null address with invalid signature

Merge branch 'main' into test-native-offer-fulfill-reverts

2f6d38b

run linter

ebee871

Merge pull request #504 from ProjectOpenSea/test-native-offer-fulfill…

57baf77

…-reverts Test that "fulfill" methods revert on Orders with Native offer items

run one final comment pass over the token transferrer

487841b

Merge pull request #505 from ProjectOpenSea/token-transferrer-comment…

71f9035

…-pass run one final comment pass over the token transferrer

include 1.1 contributors list in Seaport natspec

0b1adf3

Merge pull request #506 from ProjectOpenSea/contributors-natspec

b444a9b

include 1.1 contributors list in Seaport natspec

axic reviewed Jun 11, 2022

View reviewed changes

0age and others added 3 commits June 11, 2022 14:41

update deployment addresses

a0c99d2

include Rinkeby #code in link

fa8bee7

Merge pull request #508 from ProjectOpenSea/update-deployment-addresses

821a049

update deployment addresses

Thro77le reviewed Jun 14, 2022

View reviewed changes

code423n4 mentioned this pull request Nov 11, 2022

fulfillAdvancedOrder return value ignored code-423n4/2022-11-looksrare-findings#93

Open

github-actions bot added the Stale label May 9, 2023

0age added Informational and removed Stale labels May 10, 2023

ProjectOpenSea deleted a comment from github-actions bot May 23, 2023

0age changed the title ~~Seaport 1.0 => 1.1~~ Seaport 1.1 Jul 17, 2023

ryanio marked this pull request as draft April 16, 2024 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Seaport 1.1 #492

Seaport 1.1 #492

0age commented Jun 10, 2022

axic Jun 11, 2022

0age Jun 11, 2022

axic Jun 12, 2022

axic Jun 11, 2022

0age Jun 11, 2022

axic Jun 11, 2022

axic Jun 11, 2022

axic Jun 11, 2022 •

edited

Loading

axic Jun 11, 2022

axic Jun 11, 2022

axic Jun 11, 2022

0age Jun 11, 2022

axic Jun 11, 2022 •

edited

Loading

0age Jun 11, 2022

Thro77le Jun 14, 2022

Seaport 1.1 #492

Are you sure you want to change the base?

Seaport 1.1 #492

Conversation

0age commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Jun 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Jun 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Jun 11, 2022 •

edited

Loading

axic Jun 11, 2022 •

edited

Loading