Open code423n4 opened 2 years ago
Missing documentation for bool roundUp in docstring TokenTransferrer contract is missing contract documentation
Yes, ideally the documentation coverage would be complete - but these are very much nice to have improvements.
ERC712 transfers should use safeTransferFrom instead of transferFrom
Using safeTransferFrom is a common best practice to recommend. In this project it was intentionally avoided -- see the sponsor comment here https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/173#issuecomment-1147729846 and https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/19#issuecomment-1136460377
Should not assume the last route in else statement
The sentiment here is valid, but the current code does cover all the scenarios. If they were to make the recommended change it would be more difficult to validate it with 100% code coverage unless a mock was used to push through inputs that otherwise would not occur.
LOW 01: Missing documentation for
bool roundUp
in docstringIn AmountDeriver._applyFraction All parameters are documented properly except for
bool roundUp
which does not have an explanation in the function documentation.Recommendation
Add the parameter to the doc string;
LOW 02
TokenTransferrer
contract is missing contract documentationThe contact TokenTransferrer should have general contract documentation above the contract code, similar to all other contracts in the protocol, in such format:
LOW 03
ERC712
transfers should usesafeTransferFrom
instead oftransferFrom
Currently, the
ERC721Interface
contains onlytransferFrom
. Hence, for manyERC721
tokens which havesafeTransferFrom
as well, the non safe function will get called. By openzeppeling ERC721 standard the safe transfer is safer:I suggest the
ERC721Interface
should use thesafeTransferFrom
, and then, for example, in TokenTransferrer._performERC721Transfer the safer transfer method will get executedLOW 04: Should not assume the last
route
inelse
statementIn ReferenceBasicOrderFulfiller._validateAndFulfillBasicOrder, there are many
if-else
statements which check theroute
. the lastelse
assumes the routh is the last one left. This is problematic as the route can be not an expected value and any value will enter theelse
. Instead, I recommend to useelse if
for all values, and finish with anelse ... revert InvalidRoute()
.For example:
should be