the test suite was a joy to work with. I learned a lot about Foundry and will use it on my own projects.
the documentation was of a very high quality
Low Risk: Don't allow setVaultBeneficiary to be set to address(0)
Yes, getVaultBeneficiary will check for address(0) and return ownerOf[vaultId] but relying on this is a bit dangerous if you change the code in future.
Non-critical: Incorrect error message on check for reserve strike price
There is an incorrect error message in the require at location Cally.sol:169.
It should be:
require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too large");
In general the error message should always express the opposite of the condition of the require statement.
Non-critical: Several requires with == false in them
QA Report
Remarks/Recommendations
Low Risk: Don't allow
setVaultBeneficiaryto be set toaddress(0)Yes,
getVaultBeneficiarywill check foraddress(0)and returnownerOf[vaultId]but relying on this is a bit dangerous if you change the code in future.Non-critical: Incorrect error message on check for reserve strike price
There is an incorrect error message in the
requireat location Cally.sol:169.It should be:
In general the error message should always express the opposite of the condition of the
requirestatement.Non-critical: Several requires with
== falsein themThe locations are
It is better to use the
!(not) operator. e.g.