[WP-H3] Imprecise management of users' allowance allows the admin of the upgradeable proxy contract to rug users

[code423n4]

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.

[Zer0dot]

This is true, but is within the acceptable risk model.

[0xleastwood]

While I agree with the warden, this assumes that the admin acts maliciously. As such, I think medium risk is more in line with a faulty governance.

[Zer0dot]

I still relatively disagree with severity, as with #26 faulty governance is an acceptable risk parameter. Though we should have been more clear that this is the case!

[0xleastwood]

I agree with you fully here, but again it comes down to the judging guidelines which might change in the future. The stated attack vector has stated assumptions which leads to a loss of funds.

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.