QA Report

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/follow/FollowValidatorFollowModuleBase.sol#L23-L38

Vulnerability details

Per the comment in ApprovalFollowModule, it aims to create a whitelist system:

This follow module only allows addresses that are approved for a profile by the profile owner to follow.

However, the current implementation only validates if _approvedByProfileByOwner when minting a new followNFT. But since the follow relationship is verified by the followNFT and the followNFT can be transferred to an arbitrary address.

As a result, a non-whitelisted address can bypass the whitelist by buying the followNFT from a whitelisted address.

The followModuleTransferHook can be used for blocking transfers to unapproved addresses.

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/follow/FollowValidatorFollowModuleBase.sol#L23-L38

function validateFollow(
        uint256 profileId,
        address follower,
        uint256 followNFTTokenId
    ) external view override {
        address followNFT = ILensHub(HUB).getFollowNFT(profileId);
        if (followNFT == address(0)) revert Errors.FollowInvalid();
        if (followNFTTokenId == 0) {
            // check that follower owns a followNFT
            if (IERC721(followNFT).balanceOf(follower) == 0) revert Errors.FollowInvalid();
        } else {
            // check that follower owns the specific followNFT
            if (IERC721(followNFT).ownerOf(followNFTTokenId) != follower)
                revert Errors.FollowInvalid();
        }
    }

PoC

1. Alice created a private club and selected ApprovalFollowModule with 100 addresses of founding members and wanted to publish publications that can only be collected by those addresses;

2. Bob as founding members of Alice's private club, decides to sell his membership to Charlie by transfering the followNFT to Charlie, Charlie's address has never be approved by Alice, but he is now a follower in Alice's private club.

Recommendation

Change to:

    function followModuleTransferHook(
        uint256 profileId,
        address from,
        address to,
        uint256 followNFTTokenId
    ) external override {
        address owner = IERC721(HUB).ownerOf(profileId);
        if (!_approvedByProfileByOwner[owner][profileId][to])
            revert Errors.FollowNotApproved();
        _approvedByProfileByOwner[owner][profileId][to] = false; // prevents repeat follows
    }

[oneski]

this is by design. a more sophisticated module could implement non-transferable behavior or allow transfer only to approved addresses.

[0xleastwood]

IMO, this is a valid issue. If we want to strictly adhere to the whitelist, we should not allow whitelisted addresses to transfer NFTs to non-whitelisted addresses. This limits the formation of a secondary markets where users can bypass whitelist restrictions.

[Zer0dot]

IMO, this is a valid issue. If we want to strictly adhere to the whitelist, we should not allow whitelisted addresses to transfer NFTs to non-whitelisted addresses. This limits the formation of a secondary markets where users can bypass whitelist restrictions.

Unfortunately transferrability is the in the nature of the protocol. The module's purpose is not to limit follows to only whitelisted wallets (as this is impossible to guarantee in case follow NFTs existed previously, though one could use the validation function etc). Rather, the goal of this module is simply to only allow whitelisted users to mint follow NFTs, what they do with them is up to them.

Due to the confusing nature of the module, I would not dispute this but I would still disagree that it's a medium severity issue, I would mark it as low.

[0xleastwood]

IMO, this is a valid issue. If we want to strictly adhere to the whitelist, we should not allow whitelisted addresses to transfer NFTs to non-whitelisted addresses. This limits the formation of a secondary markets where users can bypass whitelist restrictions.

Unfortunately transferrability is the in the nature of the protocol. The module's purpose is not to limit follows to only whitelisted wallets (as this is impossible to guarantee in case follow NFTs existed previously, though one could use the validation function etc). Rather, the goal of this module is simply to only allow whitelisted users to mint follow NFTs, what they do with them is up to them.

Due to the confusing nature of the module, I would not dispute this but I would still disagree that it's a medium severity issue, I would mark it as low.

So if I'm understanding this correctly, whitelisted users who mint follow NFTs are free to do whatever with those, whether they sell them to other parties or give them away. It's completely up to them?

If that's the case, I'll agree and mark this as QA.

[Zer0dot]

IMO, this is a valid issue. If we want to strictly adhere to the whitelist, we should not allow whitelisted addresses to transfer NFTs to non-whitelisted addresses. This limits the formation of a secondary markets where users can bypass whitelist restrictions.

Unfortunately transferrability is the in the nature of the protocol. The module's purpose is not to limit follows to only whitelisted wallets (as this is impossible to guarantee in case follow NFTs existed previously, though one could use the validation function etc). Rather, the goal of this module is simply to only allow whitelisted users to mint follow NFTs, what they do with them is up to them. Due to the confusing nature of the module, I would not dispute this but I would still disagree that it's a medium severity issue, I would mark it as low.

So if I'm understanding this correctly, whitelisted users who mint follow NFTs are free to do whatever with those, whether they sell them to other parties or give them away. It's completely up to them?

If that's the case, I'll agree and mark this as QA.

Yeah although it's valid enough, this is just how the module works.

[0xleastwood]

IMO, this is a valid issue. If we want to strictly adhere to the whitelist, we should not allow whitelisted addresses to transfer NFTs to non-whitelisted addresses. This limits the formation of a secondary markets where users can bypass whitelist restrictions.

Unfortunately transferrability is the in the nature of the protocol. The module's purpose is not to limit follows to only whitelisted wallets (as this is impossible to guarantee in case follow NFTs existed previously, though one could use the validation function etc). Rather, the goal of this module is simply to only allow whitelisted users to mint follow NFTs, what they do with them is up to them. Due to the confusing nature of the module, I would not dispute this but I would still disagree that it's a medium severity issue, I would mark it as low.

So if I'm understanding this correctly, whitelisted users who mint follow NFTs are free to do whatever with those, whether they sell them to other parties or give them away. It's completely up to them?

If that's the case, I'll agree and mark this as QA.

Yeah although it's valid enough, this is just how the module works.

Okay, QA it is good ser

[liveactionllama]

Per conversation with the judge ( @0xleastwood ), confirmed this issue is categorized as low risk.