Aave Lens contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/collect/FeeCollectModule.sol#L163-L172

Vulnerability details

In the current implementation, even when the profile's owner burnt the ProfileNFT, as the profile's legacy, the publications can still be collected.

However, if the publication is a Mirror and there is a referralFee set by the original publication, the user won't be able to collect from a Mirror that was published by a burned profile.

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/collect/FeeCollectModule.sol#L163-L172

if (referralFee != 0) {
    // The reason we levy the referral fee on the adjusted amount is so that referral fees
    // don't bypass the treasury fee, in essence referrals pay their fair share to the treasury.
    uint256 referralAmount = (adjustedAmount * referralFee) / BPS_MAX;
    adjustedAmount = adjustedAmount - referralAmount;

    address referralRecipient = IERC721(HUB).ownerOf(referrerProfileId);

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

When a mirror is collected, what happens behind the scenes is the original, mirrored publication is collected, and the mirror publisher's profile ID is passed as a "referrer."

In _processCollectWithReferral(), if there is a referralFee, contract will read referralRecipient from IERC721(HUB).ownerOf(referrerProfileId), if referrerProfileId is burned, the IERC721(HUB).ownerOf(referrerProfileId) will revert with ERC721: owner query for nonexistent token.

However, since we wish to allow the content to be collected, we should just treat referrals as non-existent in this situation.

Recommendation

Change to:

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

    address referralRecipient = IERC721(HUB).ownerOf(referrerProfileId);

    IERC20(currency).safeTransferFrom(collector, referralRecipient, referralAmount);
} catch {
    emit LogNonExistingReferrer(referrerProfileId);
}

1. Stop displaying all the burnt profile's publications
2. Redirect users, when collecting mirrors, to the original publication
3. Prevent all collects

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/follow/FeeFollowModule.sol#L75-L91

Vulnerability details

In the current implementation, when there is a fee on follow or collect, users need to approve to the follow modules or collect module contract, and then the Hub contract can call processFollow() and transfer funds from an arbitrary address (as the follower parameter).

https://github.com/code-423n4/2022-02-aave-lens/blob/c1d2de2b0609b7d2734ada2ce45c91a73cc54dd9/contracts/core/modules/follow/FeeFollowModule.sol#L75-L91

function processFollow(
    address follower,
    uint256 profileId,
    bytes calldata data
) external override onlyHub {
    uint256 amount = _dataByProfile[profileId].amount;
    address currency = _dataByProfile[profileId].currency;
    _validateDataIsExpected(data, currency, amount);

    (address treasury, uint16 treasuryFee) = _treasuryData();
    address recipient = _dataByProfile[profileId].recipient;
    uint256 treasuryAmount = (amount * treasuryFee) / BPS_MAX;
    uint256 adjustedAmount = amount - treasuryAmount;

    IERC20(currency).safeTransferFrom(follower, recipient, adjustedAmount);
    IERC20(currency).safeTransferFrom(follower, treasury, treasuryAmount);
}

A common practice is asking users to approve an unlimited amount to the contracts. Normally, the allowance can only be used by the user who initiated the transaction.

It's not a problem as long as transferFrom will always only transfer funds from msg.sender and the contract is non-upgradable.

However, the LensHub contract is upgradeable, and FeeFollowModule will transferFrom an address decided by an input parameter follower, use of upgradeable proxy contract structure allows the logic of the contract to be arbitrarily changed.

This allows the proxy admin to perform many malicious actions, including taking funds from users' wallets up to the allowance limit.

This action can be performed by the malicious/compromised proxy admin without any restriction.

PoC

Given:

• profileId 1 uses FeeCollectModule with currency = WETH and amount = 1e18
• Alice is rich (with 1,000,000 WETH in wallet balance)

1. Alice approve() type(uint256).max to FeeCollectModule and follow() profileId 1 with 1e18 WETH;
2. A malicious/compromised proxy admin can call upgradeToAndCall() on the proxy contract and set a malicious contract as newImplementation, adding a new functions:

function rugFollow(address follower, address followModule, uint256 profileId, bytes calldata data)
    external
{
    IFollowModule(followModule).processFollow(
        follower,
        profileIds,
        data
    );
}

3. The attacker creates a new profile with FeeCollectModule and set currency = WETH and amount = 1,000,000, got profileId = 2
4. The attacker rugFollow() with follower = Alice, profileId = 2, stolen 1,000,000 WETH from Alice's wallet

Recommendation

Improper management of users' allowance is the root cause of some of the biggest attacks in the history of DeFi security. We believe there are 2 rules that should be followed:

1. the from of transferFrom should always be msg.sender;
2. the contracts that hold users' allowance should always be non-upgradable.

We suggest adding a non-upgradeable contract for processing user payments:

function followWithFee(uint256 profileId, bytes calldata data)
    external
{
    (address currency, uint256 amount) = abi.decode(data, (address, uint256));
    IERC20(currency).transferFrom(msg.sender, HUB, amount);

    uint256[] memory profileIds = new uint256[](1);
    profileIds[0] = profileId;

    bytes[] memory datas = new bytes[](1);
    datas[0] = data;

    IHUB(HUB).follow(profileIds, datas);
}

This comes with an additional benefit: users only need to approve one contract instead of approving multiple follow or collect module contracts.

2 — Med (M): vulns have a risk of 2 and are considered “Medium” severity when assets are not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

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
    }