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

1 stars 0 forks source link

QA Report #85

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Floating pragma and different compiler versions among contracts

Some of the contracts are using the floating pragma of >=0.8.7 and others use 0.8.13 Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. It is also recommended to use the same solidity version for all the contracts.

References: https://swcregistry.io/docs/SWC-103 https://github.com/crytic/slither/wiki/Detector-Documentation#different-pragma-directives-are-used

Events missing indexed fields

Each event can have up to 3 indexed fields.

  1. https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/interfaces/ConduitControllerInterface.sol#L29
    event NewConduit(address conduit, bytes32 conduitKey);  //@audit address conduit can be made indexed
  2. https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/interfaces/ConduitInterface.sol#L48
    event ChannelUpdated(address channel, bool open);       //@audit address channel can be made indexed
  3. https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/interfaces/ConsiderationEventsAndErrors.sol#L25-L32
    event OrderFulfilled(
        bytes32 orderHash,
        address indexed offerer,
        address indexed zone,
        address fulfiller,                  //@audit address fulfiller can be made indexed
        SpentItem[] offer,
        ReceivedItem[] consideration
    );
0age commented 2 years ago

need pragma for reference contracts, indexed events are more expensive and these values are not going to be used repeatedly

HardlyDifficult commented 2 years ago

Floating compiler versions

Their config pins the version used, so this is a minor style preference. I often using floating personally so that when I upgrade versions the diff is just the config and therefore easy to review.

event is missing indexed fields

There is a cost to using indexed, so it should only be added where it will add value. It's not indicated which fields would be appropriate to index here.

Since none of these are clear wins, closing as invalid.

GalloDaSballo commented 2 years ago

Agree with Judge and Sponsor, but would consider indexing the Conduit on creation as that could be arguably used to then list to future events on it.

Because of that I think 1 NC is valid from this report

GalloDaSballo commented 2 years ago

2 NC per updated judging on #67