QA Report

[code423n4]

QA (LOW & NON-CRITICAL)

• The whole project have different solidity compiler ranges (>0.8.7, 0.8.13) referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior.

• There is no address(0) check in Seaport.sol's constructor() function

• At ConduitController.sol, createConduit() function there is no address(0) check and 0 value check for the supplied parameters.

• At ConduitController.sol, createConduit() function, the new conduit contract is created via Create2 function. However, the created conduit and the calculated address is not compared against each other. Reference

Conduit c = new Conduit{ salt: conduitKey }();
require (address(conduit) == address(c), "err");

• Critical address changes are not done in two steps for the followings:
  At ConduitController.sol, createConduit()

Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible. Reference, Reference

• At ConduitController.sol, getChannel() function, the logic should be;

if (channelIndex <= totalChannels) {
            revert ChannelOutOfRange(conduit);
        }

rather than;

if (channelIndex >= totalChannels) {
            revert ChannelOutOfRange(conduit);
        }

• At AmountDeriver.sol's _getFraction() function, NatSpec states that the divisor should not be zero. However, it's not implemented in contract level by a require statement.

• ConduitController.sol, BasicOrderFulfiller.sol, ConsiderationBase.sol, ReferenceConduitController.sol, use abi.encodePacked(). Using abi.encodePacked() with multiple variable length arguments can, in certain situations, lead to a hash collision. Instead abi.encode() can be used. Reference Link

GAS OPTIMIZATIONS

• Event emitted from the STORAGE at OwnershipTransferred() in ConduitController.sol

[GalloDaSballo]

The whole project have different solidity compiler ranges (>0.8.7, 0.8.13)

Disagree per discussion on #65

Zero-checks in general / constructor / functions

Disagree for sake of gas savings

At ConduitController.sol, createConduit() function there is no address(0) check and 0 value check for the supplied parameters.

Agree because of #56
Low valid

At ConduitController.sol, createConduit() function is not compared

I believe the code to be fine, however had the warden demonstrated the actual inconsistency with a POC (address != estimate) then the finding could have been upgraded to a higher severity. In lack of that I believe this to be invalid

At ConduitController.sol, getChannel() function, the logic should be;

@0xleastwood @HardlyDifficult @0age this looks valid, wdyt?

Disagree with rest

1 or 2 Low Valid based on Sponsor Feedback

[HardlyDifficult]

At ConduitController.sol, getChannel() function, the logic should be;

The current code is correct. For example, let's say 3 channels were created and getChannel(1) is called. The proposal is if (1 <= 3) revert which should not revert.

[GalloDaSballo]

Updated #67 so

1L + 1 NC