-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
QA Report #60
Comments
Again another great report from Dravee! To the judge-- it's possible I may have previously denied the validity of the zero address checks, note that I now believe there's virtually no reason not to implement them, so they are valid. However, the init frontrun issue is invalid because collect & follow NFTs are initialized in the same transaction as their cloning from the hub. The ERC721Enumerable changes I'm not 100% sure on, this is basically CC'd from OpenZeppelin, I'll have to ask for clarity on that @miguelmtzinf @donosonaumczuk @tabshaikh if you guys want to weigh in. Everything's updated here: lens-protocol/core#80 |
I'm not following Dravee on the ERC721Enumerable stuff.
This is not necessary, as
Both |
Nicely done @donosonaumczuk thanks for the input! We won't change anything in the ERC721 implementations forked from OZ. |
Awesome writeup, most of these are completely valid. I think the issue outlined in #53 and mentioned here does not point to an issue that may occur. Additionally, the frontrun issue is really only applicable to non-proxy setups which do not initialize and deploy in the same transaction. Fortunately, proxies do this safely. |
QA Report
Table of Contents:
hub
initialize
can be called by everyone and front-runhub
initialize
can be called by everyone and front-runfollowNFTImpl
collectNFTImpl
initialize
can be called by everyone and front-runForeword
@audit
tagsFile: CollectNFT.sol
constructor(address hub)
Missing Address(0) check on
hub
address hub
should be address(0) checked.This type of address(0) check is already done in the solution (even for
hub
), see inModuleBase.sol
:I suggest doing the same as in
ModuleBase.sol
.function initialize()
initialize
can be called by everyone and front-runThe
initialize
function is missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.I recommend adding some type of access control (onlyHub? onlyGov?) to
initialize()
.File: FollowNFT.sol
constructor(address hub)
Missing Address(0) check on
hub
See Missing Address(0) check on
hub
for a similar explanationfunction initialize()
initialize
can be called by everyone and front-runSee
initialize
can be called by everyone and front-run for a similar explanationFile: LensHub.sol
constructor(address followNFTImpl, address collectNFTImpl)
Missing Address(0) check on
followNFTImpl
Missing Address(0) check on
collectNFTImpl
See Missing Address(0) check on
hub
for a similar explanationfunction initialize()
initialize
can be called by everyone and front-runSee
initialize
can be called by everyone and front-run for a similar explanationFile: ERC721Enumerable.sol
function _addTokenToAllTokensEnumeration(uint256 tokenId)
_allTokensIndex[tokenId] should be checked for existence
This issue was already submitted as a medium issue. Mentioning it in the QA-Report still felt valuable. To avoid pushing multiple times a
tokenId
in the array and breaking the logic, _allTokensIndex[tokenId] should be checked for existence.function _removeTokenFromOwnerEnumeration(address from, uint256 tokenId)
Missing pop()
The comments say that a swap & pop should happen in this function.
However, this isn't the case:
I suggest either correcting the comment or doing like in
_removeTokenFromAllTokensEnumeration()
:File: ERC721Time.sol
function _checkOnERC721Received() private
Wrong comment
should be
This would follow the practice from other places like ERC721Enumerable at L110, L121, L130 and L158:
File: LimitedFeeCollectModule.sol
function initializePublicationCollectModule()
Missing comment: @param profileId
Missing comment: @param pubId
File: LimitedTimedFeeCollectModule.sol
function initializePublicationCollectModule()
Missing comment: @param profileId
Missing comment: @param pubId
File: TimedFeeCollectModule.sol
function initializePublicationCollectModule()
Missing comment: @param profileId
Missing comment: @param pubId
File: ApprovalFollowModule.sol
function initializeFollowModule()
Missing comment: @param profileId
function isApproved()
Incomplete @return definition (no description)
function isApprovedArray()
Missing comment: @return bool[]
File: FeeFollowModule.sol
function initializeFollowModule()
Missing comment: @param profileId
File: IFollowModule.sol
function initializeFollowModule()
Missing comment: @return bytes
It should be:
@return An abi encoded bytes parameter, which is the same as the passed data parameter.
.File: IFollowNFT.sol
function getPowerByBlockNumber()
Missing comment: @return uint256
function getDelegatedSupplyByBlockNumber()
Missing comment: @return uint256
File: ILensHub.sol
function getProfile()
Missing comment: @return DataTypes.ProfileStruct
File: Events.sol
event ProfileCreated()
Missing comment: @param followNFTURI
File: InteractionLogic.sol
function follow()
Missing comment: @param _profileIdByHandleHash
The text was updated successfully, but these errors were encountered: