Skip to content
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 #80

Open
code423n4 opened this issue Feb 16, 2022 · 1 comment
Open

QA Report #80

code423n4 opened this issue Feb 16, 2022 · 1 comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

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

@code423n4 code423n4 added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax bug Something isn't working labels Feb 16, 2022
code423n4 added a commit that referenced this issue Feb 16, 2022
@Zer0dot
Copy link
Collaborator

Zer0dot commented Mar 21, 2022

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)

@Zer0dot Zer0dot added sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons and removed sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

2 participants