code-423n4 / 2022-02-aave-lens-findings

0 stars 0 forks source link

QA Report #80

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

LensHub constructor lack input validation

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L57 Recommended to add check to make sure addresses supplied is not address(0)

CollectNFT constructor lack input validation

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/CollectNFT.sol#L29 Recommended to add check to make sure addresses supplied is not address(0)

FollowNFT constructor lack input validation

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/FollowNFT.sol#L48 Recommended to add check to make sure addresses supplied is not address(0)

Interdependency of LensHub and CollectNFT/FollowNFT construction

CollectNFT/FollowNFT require the address of LensHub in the constructor, while LensHub require the address of CollectNFT and FollowNFT. While we can use a pre-computed address, it might be better to call the initializer from LensHub to set everything at once.

LensHub.initialize lack input validation

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L63 Recommended to add check to make sure newGovernance is not address(0)

Add comment to explain why ++pubCount is not used

The way _createComment coded is likely to prevent comment loop so pubCount is incremented after PublishingLogic.createComment. It looks a bit werid since other function use ++pubCount instead. Recommed to add soe comment here for future reference. https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L878

    function _createComment(DataTypes.CommentData memory vars) internal {
        PublishingLogic.createComment(
            vars,
            _profileById[vars.profileId].pubCount + 1,
            _profileById,
            _pubByIdByProfile,
            _collectModuleWhitelisted,
            _referenceModuleWhitelisted
        );
        _profileById[vars.profileId].pubCount++;
    }

Implement 2 steps governance transfer in LensHub

To reduce risk of failed governance transfer, use a 2 steps process that require the new governance's interaction. https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/LensHub.sol#L78

Implement 2 steps governance transfer in ModuleGlobals

To reduce risk of failed governance transfer, use a 2 steps process that require the new governance's interaction. https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/ModuleGlobals.sol#L50

Zer0dot commented 2 years ago

Various duplicates, we are not acting on any of this except the comment, which is a good point. On the point of input validation, this is true but the constructors and the LensHub initializer, being part of the initial setup, are within the trust assumptions (just like governance being a legitimate timelock controlled by a multisig etc)