OpenSea Seaport 1.2 contest - btk's results

Total Low issues

Total Non-Critical issues

[L-01] Low level calls with solidity version 0.8.14 and lower can result in optimiser bug

The protocol is using low level calls with solidity version less then 0.8.14 which can result in optimizer bug.

Ref: https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d

Lines of code

• TokenTransferrer.sol
• ConsiderationStructs.sol
• ConsiderationEncoder.sol
• PointerLibraries.sol
• Conduit.sol

Recommended Mitigation Steps

Consider upgrading to solidity 0.8.17

[L-02] Address(0) checks

Check of address(0) to protect the code from (0x0000000000000000000000000000000000000000) address problem just in case. This is best practice or instead of suggesting that they verify _address != address(0), you could add some good NatSpec comments explaining what is valid and what is invalid and what are the implications of accidentally using an invalid address.

        constructor(address conduitController) Consideration(conduitController) {}

Lines of code

• Seaport.sol:93
• Assertions.sol:36
• BasicOrderFulfiller.sol:41
• Consideration.sol:50
• ConsiderationBase.sol:56
• Executor.sol:35
• GettersAndDerivers.sol:26
• OrderCombiner.sol:41
• OrderFulfiller.sol:46
• OrderValidator.sol:55
• Verifiers.sol:26

[L-03] Multiple Pragma used

Solidity pragma versions should be exactly same in all contracts. Currently some contracts use ^0.8.13 and others use ^0.8.17

Lines of code

• All Contracts

Recommended Mitigation Steps

Use one version for all contracts.

[NC-01] Include @return parameters in NatSpec comments

If Return parameters are declared, you must prefix them with /// @return. Some code analysis programs do analysis by reading NatSpec details, if they can't see the @return tag, they do incomplete analysis.

Lines of code

• All Contracts

Recommended Mitigation Steps

Include the @return argument in the NatSpec comments.

[NC-02] Contracts should have full test coverage

While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.

- What is the overall line coverage percentage provided by your tests?: >90%

Lines of code

• All Contracts

Recommended Mitigation Steps

Line coverage percentage should be 100%.

[NC-03] Need Fuzzing test

In total 14 contracts, more than 54 unchecked{} are used, the functions used are critical. For this reason, there must be fuzzing tests in the tests of the project. Not seen in tests.

Lines of code

• All Contracts

Recommended Mitigation Steps

Use should fuzzing test like Echidna. As Alberto Cuesta Canada said: Fuzzing is not easy, the tools are rough, and the math is hard, but it is worth it. Fuzzing gives me a level of confidence in my smart contracts that I didn’t have before. Relying just on unit testing anymore and poking around in a testnet seems reckless now.

Ref: https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05

[NC-04] Use a more recent version of solidity

For security, it is best practice to use the latest Solidity version. For the security fix list in the versions.

https://github.com/ethereum/solidity/blob/develop/Changelog.md

       pragma solidity ^0.8.13;

Lines of code

• ConsiderationConstants.sol:2
• TokenTransferrer.sol:2
• ConsiderationStructs.sol:2
• ConsiderationEnums.sol:2
• ConsiderationEncoder.sol:2
• ConsiderationConstants.sol:2
• ZoneInterface.sol:2
• ZoneInteractionErrors.sol:2
• TransferHelperInterface.sol:2
• TransferHelperErrors.sol:2
• TokenTransferrerErrors.sol:2
• SignatureVerificationErrors.sol:2
• SeaportInterface.sol:2
• ReentrancyErrors.sol:2
• ImmutableCreate2FactoryInterface.sol:2
• IERC721Receiver.sol:2
• FulfillmentApplicationErrors.sol:2
• EIP1271Interface.sol:2
• CriteriaResolutionErrors.sol:2
• ContractOffererInterface.sol:2
• ConsiderationInterface.sol:2
• ConsiderationEventsAndErrors.sol:2
• ConduitInterface.sol:2
• ConduitControllerInterface.sol:2
• AmountDerivationErrors.sol:2
• AbridgedTokenInterfaces.sol:2
• PointerLibraries.sol:2
• ConduitStructs.sol:2
• ConduitEnums.sol:2
• ConduitConstants.sol:2
• ConduitController.sol:2
• Conduit.sol:2

Recommended Mitigation Steps

Old version of Solidity is used (^0.8.13), newer version can be used (0.8.17).

[NC-05] Missing natspec

It is recommended that Solidity contracts are fully annotated using NatSpec, it is clearly stated in the Solidity official documentation.

• In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

• Some code analysis programs do analysis by reading NatSpec details, if they can't see the tags (@param, @dev, @return), they do incomplete analysis.

Lines of code

• ConsiderationStructs.sol:286
• PointerLibraries.sol

Recommended Mitigation Steps

Include NatSpec comments in the codebase.

[NC-06] Constants in comparisons should appear on the left side

Constants in comparisons should appear on the left side, doing so will prevent typo bug.

       if (maximumFulfilled == 0) {

Lines of code

• OrderCombiner.sol:237
• OrderCombiner.sol:256
• OrderCombiner.sol:639
• OrderCombiner.sol:741
• FulfillmentApplier.sol:56
• FulfillmentApplier.sol:75
• FulfillmentApplier.sol:177
• FulfillmentApplier.sol:211
• Executor.sol:298
• Executor.sol:346
• Executor.sol:402

Recommended Mitigation Steps

Constants should appear on the left side.

       if (0 == maximumFulfilled) {

[NC-07] Use bytes.concat() and string.concat()

• bytes.concat() vs abi.encodePacked(<bytes>,<bytes>)
• string.concat() vs abi.encodePacked(<string>,<string>)

https://docs.soliditylang.org/en/v0.8.17/types.html?highlight=bytes.concat#the-functions-bytes-concat-and-string-concat

Lines of code

• ConduitController.sol:76
• ConduitController.sol:339
• TypehashDirectory.sol:177

Recommended Mitigation Steps

Use bytes.concat() and string.concat().

[NC-08] Add I prefix to interface contracts

Filename prefixed by I usually indicates an interface rather than a contract.

Lines of code

• All Interfaces

Recommended Mitigation Steps

Add I prefix to interface contracts

[NC-09] Generate perfect code headers every time

I recommend using header for Solidity code layout and readability

Ref: https://github.com/transmissions11/headers

Lines of code

• All Contracts

Recommended Mitigation Steps

/*//////////////////////////////////////////////////////////////
                           TESTING 123
//////////////////////////////////////////////////////////////*/

[NC-10] Lock pragmas to specific compiler version

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile locally.

Ref: https://swcregistry.io/docs/SWC-103

       pragma solidity ^0.8.13;

       pragma solidity ^0.8.17;

Lines of code

• All Contracts

Recommended Mitigation Steps

Ethereum Smart Contract Best Practices - Lock pragmas to specific compiler version.

https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

[NC-11] There is no need to cast a variable that is already an address, such as uint(x)

There is no need to cast a variable that is already an uint, such as uint(0), 0 is also uint.

       if (identifierOrCriteria != uint256(0)) {

Lines of code

• CriteriaResolution.sol:212
• Executor.sol:310

Recommended Mitigation Steps

Use directly variable.

       if (identifierOrCriteria != 0) {

[NC-12] Consider using delete rather than assigning zero to clear values

The delete keyword more closely matches the semantics of what is being done, and draws more attention to the changing of state, which may lead to a more thorough audit of its associated logic

       advancedOrder.numerator = 0;

Lines of code

• OrderCombiner.sol:239
• OrderCombiner.sol:258

Recommended Mitigation Steps

Use the delete keyword.

       delete advancedOrder.numerator;

[NC-13] Assembly Codes Specific – Should Have Comments

Since this is a low level language that is more difficult to parse by readers, include extensive documentation, comments on the rationale behind its use, clearly explaining what each assembly instruction does

This will make it easier for users to trust the code, for reviewers to validate the code, and for developers to build on or update the code.

Note that using Aseembly removes several important security features of Solidity, which can make the code more insecure and more error-prone.

Lines of code

• PointerLibraries.sol:52
• PointerLibraries.sol:61
• PointerLibraries.sol:70
• ConsiderationBase.sol:274
• ConsiderationDecoder.sol:563-594
• ConsiderationStructs.sol:286-542

Recommended Mitigation Steps

Include NatSpec in assembly codes.

[NC-14] Adding a return statement when the function defines a named return variable, is redundant

       return amount;

Lines of code

• AmountDeriver.sol:87
• BasicOrderFulfiller.sol:902
• FulfillmentApplier.sol:143
• OrderCombiner.sol:651
• OrderCombiner.sol:1047