Aave Lens contest - cccz's results

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/FeeCollectModule.sol#L139-L177

Vulnerability details

Impact

In the *FeeCollectModule contract, the recipient of the fee is specified by the user when the post is created, that is, even if the profileNFT is transferred or destroyed, the fee will still be sent to the address specified by the user when the post is created.

    function initializePublicationCollectModule(
        uint256 profileId,
        uint256 pubId,
        bytes calldata data
    ) external override onlyHub returns (bytes memory) {
        ...
        _dataByPublicationByProfile[profileId][pubId].recipient = recipient;

When collecting the mirror, the receiver of the referralFee is the owner of the referrer's profileNFT, that is, the referralFee will be sent to different addresses along with the transfer of the referrer's profileNFT.

        if (referralFee != 0) {
            uint256 referralAmount = (adjustedAmount * referralFee) / BPS_MAX;
            adjustedAmount = adjustedAmount - referralAmount;
            address referralRecipient = IERC721(HUB).ownerOf(referrerProfileId);

            IERC20(currency).safeTransferFrom(collector, referralRecipient, referralAmount);
        }

When profileNFT is destroyed, require(owner != address(0) in the ownerOf function will fail, resulting in DOS.

    function _burn(uint256 tokenId) internal virtual {
        address owner = ERC721Time.ownerOf(tokenId);

        _beforeTokenTransfer(owner, address(0), tokenId);

        // Clear approvals
        _approve(address(0), tokenId);

        _balances[owner] -= 1;
        delete _tokenData[tokenId];

        emit Transfer(owner, address(0), tokenId);
    }
...
    function ownerOf(uint256 tokenId) public view virtual override returns (address) {
        address owner = _tokenData[tokenId].owner;
        require(owner != address(0), 'ERC721: owner query for nonexistent token');
        return owner;
    }

Proof of Concept

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/ERC721Time.sol#L365-L377

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/ERC721Time.sol#L84-L88

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/FeeCollectModule.sol#L139-L177

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/LimitedFeeCollectModule.sol#L157-L195

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/LimitedTimedFeeCollectModule.sol#L168-L206

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/collect/TimedFeeCollectModule.sol#L153-L191

Tools Used

None

Recommended Mitigation Steps

Specify referralRecipient when creating the mirror or check if the referrer's profileNFT is destroyed when collecting the mirror

Summary

The Aave Lens code base can be roughly divided into two parts. The first part is the code used to build the social graph based on the LensHub contract. The main logic of this part is completed by the InteractionLogic and PublishingLogic libraries. The second part is the code in the modules directory, which is used to customize the needs of different users in the social graph. The security problems in the first part mainly occur in the edge cases when the user interacts, and these edge cases are also handled as much as possible in the code, including the impact of profileNFT transfer or destruction, and the nesting of post and comment and mirror. The security issues in the second part are mainly whether the customized module function is correct and whether it can be bypassed. Since more modules will be added in the future and most modules cannot be changed once they are used, the code of each new module needs to be are strictly audited.

Feature discussion

When disabling a vulnerable module, should it suspend use of the module's collect, follow, and refer functions? Take the vulnerable SecretCodeFollowModule as an example, _governance added the SecretCodeFollowModule to the whitelist, and many users used the module, then _governance realized that the SecretCodeFollowModule was vulnerable and removed the module from the whitelist, but The follow function of users using the vulnerable module will still work. Especially when the module has a critical vulnerability, preventing the continued use of the module seems to prevent the vulnerability from escalating. It seems to be a battle of security and decentralization.

Low Risk Findings

Missing 0 address check

impact

The setGovernance and setEmergencyAdmin functions of the LensHub contract are missing the check if the parameter is a 0 address.

    function setGovernance(address newGovernance) external override onlyGov {
        _setGovernance(newGovernance);
    }
    function _setGovernance(address newGovernance) internal {
        address prevGovernance = _governance;
        _governance = newGovernance;
        emit Events.GovernanceSet(msg.sender, prevGovernance, newGovernance, block.timestamp);
    }
    /// @inheritdoc ILensHub
    function setEmergencyAdmin(address newEmergencyAdmin) external override onlyGov {
        address prevEmergencyAdmin = _emergencyAdmin;
        _emergencyAdmin = newEmergencyAdmin;
        emit Events.EmergencyAdminSet(
            msg.sender,
            prevEmergencyAdmin,
            newEmergencyAdmin,
            block.timestamp
        );
    }

Proof of Concept

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L78-L92

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L850-L854

Recommended Mitigation Steps

Add 0 address check

_setGovernance should be two step process

impact

The current governanceship transfer process involves the current governance calling setGovernance(). This function write the new governance's address into the _governance state variable. If the nominated EOA account is not a valid account, it is entirely possible the governance may accidentally transfer governanceship to an uncontrolled account, breaking all functions with the onlyGov() modifier.

Proof of Concept

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L850-L854

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/ModuleGlobals.sol#L94-L99

Recommended Mitigation Steps

Implement zero address check and consider implementing a two step process where the governance nominates an account and the nominated account needs to call an acceptGovernance() function for the transfer of governanceship to fully succeed. This ensures the nominated EOA account is a valid and active account.

FRONT-RUNNABLE INITIALIZERS

impact

In the LensHub contract, the initialize function was 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.

    function initialize(
        string calldata name,
        string calldata symbol,
        address newGovernance
    ) external override initializer {
        super._initialize(name, symbol);
        _setState(DataTypes.ProtocolState.Paused);
        _setGovernance(newGovernance);
    }

Proof of Concept

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L63-L71

Recommended Mitigation Steps

Setting the _governance in the contract's constructor to the msg.sender and adding the onlyGov modifier to all initializers

No event was emitted while setting FOLLOW_NFT_IMPL and COLLECT_NFT_IMPL in constructor

impact

Event should be emitted while setting FOLLOW_NFT_IMPL and COLLECT_NFT_IMPL in constructor

    constructor(address followNFTImpl, address collectNFTImpl) {
        FOLLOW_NFT_IMPL = followNFTImpl;
        COLLECT_NFT_IMPL = collectNFTImpl;
    }

Proof of Concept

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L57-L60

Recommended Mitigation Steps

event should be emitted after the sensitive action

Non-Critical Findings

Missing 0 address check

impact

LensHub contract's whitelist* functions are missing 0 address checks

    function whitelistProfileCreator(address profileCreator, bool whitelist)
        external
        override
        onlyGov
    {
        _profileCreatorWhitelisted[profileCreator] = whitelist;
        emit Events.ProfileCreatorWhitelisted(profileCreator, whitelist, block.timestamp);
    }

    /// @inheritdoc ILensHub
    function whitelistFollowModule(address followModule, bool whitelist) external override onlyGov {
        _followModuleWhitelisted[followModule] = whitelist;
        emit Events.FollowModuleWhitelisted(followModule, whitelist, block.timestamp);
    }

    /// @inheritdoc ILensHub
    function whitelistReferenceModule(address referenceModule, bool whitelist)
        external
        override
        onlyGov
    {
        _referenceModuleWhitelisted[referenceModule] = whitelist;
        emit Events.ReferenceModuleWhitelisted(referenceModule, whitelist, block.timestamp);
    }

    /// @inheritdoc ILensHub
    function whitelistCollectModule(address collectModule, bool whitelist)
        external
        override
        onlyGov
    {
        _collectModuleWhitelisted[collectModule] = whitelist;
        emit Events.CollectModuleWhitelisted(collectModule, whitelist, block.timestamp);
    }

Proof of Concept

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L102-L135

Recommended Mitigation Steps

Add 0 address check