Open code423n4 opened 2 years ago
Disagree per discussion on #65
Disagree for sake of gas savings
Agree because of #56 Low valid
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
@0xleastwood @HardlyDifficult @0age this looks valid, wdyt?
Disagree with rest
1 or 2 Low Valid based on Sponsor Feedback
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.
Updated #67 so
1L + 1 NC
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. ReferenceCritical 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;rather than;
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
, useabi.encodePacked()
. Usingabi.encodePacked()
with multiple variable length arguments can, in certain situations, lead to a hash collision. Insteadabi.encode()
can be used. Reference LinkGAS OPTIMIZATIONS
OwnershipTransferred()
in ConduitController.sol