Description

For several types of actions, fees are paid to a relayer and the fee manager (as a service fee). In most cases, the fees are calculated by the following function:

function calculateFee(
    uint256 amount,
    uint256 relayerRefund
) external view returns (uint256, uint256, uint256) {
    uint256 serviceFeePercent = _serviceFeePercent;
    uint256 serviceCharge = (amount * serviceFeePercent) / _PRECISION;

!     require(
!         amount > serviceCharge + relayerRefund,
!         "FeeManager: amount must be greater than fees"
!     );

    return (
        amount - serviceCharge - relayerRefund,
        serviceCharge,
        relayerRefund
    );
}

Note that the serviceCharge is calculated from amount (as a percentage), while relayerRefund is an absolute amount passed as an argument. The function checks that amount is sufficient to cover both fees.

In some instances, multiple assets are involved and the following function is used instead:

function calculateFee(
    uint256[4] calldata amount,
    uint256[4] calldata relayerRefund
)
    external
    view
    returns (uint256[4] memory, uint256[4] memory, uint256[4] memory)
{
    uint256 serviceFeePercent = _serviceFeePercent;
    uint256[4] memory serviceCharge;
    uint256[4] memory actualAmount;
    for (uint256 i = 0; i < 4; i++) {
        if (amount[i] != 0) {
            serviceCharge[i] = (amount[i] * serviceFeePercent) / _PRECISION;
            require(
                amount[i] > serviceCharge[i] + relayerRefund[i],
                "FeeManager: amount must be greater than fees"
            );
            actualAmount[i] =
                amount[i] -
                serviceCharge[i] -
                relayerRefund[i];
        }
    }

    return (actualAmount, serviceCharge, relayerRefund);
}

While the amount[i] > serviceCharge[i] + relayerRefund[i] requirement is present in this variant of the function as well, it is skipped if amount[i] == 0. Thus, if amount[i] = 0 and relayerRefund[i] is an arbitrary value, then calling this variant, calculateFee, will not revert.

A function calculateFeeForFSN also exists with identical behavior, having the same issue.

The two fee calculation functions with the above bug are used in several Curve integration asset managers.

As an example, we consider the curveAddLiquidity function of the CurveAddLiquidityAssetManager contract. That function receives an AddLiquidityArgs struct as one argument, containing a field uint256[4] gasRefund. The following extract of the implementations shows the only two places where gasRefund is used:

function curveAddLiquidity(
    bytes calldata _proof,
    AddLiquidityArgs calldata _args
) external payable {

    // ...

    uint256[4] memory actualAmounts;
    uint256[4] memory serviceFees;

    (actualAmounts, serviceFees, ) = IFeeManager(_feeManager).calculateFee(
        _args.amounts,
        _args.gasRefund
    );

    uint256 mintAmount = _addLiquidity(_args, actualAmounts);

    IERC20(_args.lpToken).safeTransfer(
        address(_assetPoolERC20),
        mintAmount
    );

    _transferFees(
        _args.assets,
        serviceFees,
        _args.gasRefund,
        address(_feeManager),
        _args.relayer
    );

    // ...
}

If a relayer calls curveAddLiquidity with arguments that satisfy the following,

1. The relayer is set as _args.relayer.

2. For some i, it holds that _args.amounts[i] = 0, and _args.gasRefund[i] = a for some arbitrary a.

3. The arguments are otherwise valid.

then the call will not revert, and the relayer will receive a of _args.assets[i] as fees, as long as the asset manager held a sufficient amount of these tokens. Relayers can thus use this to steal all tokens that are held by the asset manager. In normal operation, the asset manager should not hold any tokens at rest however, so practical impact is limited to any tokens that may have been stuck by user error or other bugs.

Impact

Relayers can drain several Curve asset managers of any tokens or Ether they hold. As the asset managers usually do not hold any assets themselves, impact is limited unless attackers can combine this with another bug that leads to tokens being stuck in the asset manager (rather than being held by the relevant asset pool).

Recommendations

As future code changes that are seemingly unrelated to the FeeManager contract (e.g., making relayer fees paid directly by the asset pools rather than transferring first to the asset manager and then having the asset manager pay the relayer) could easily result in a critical vulnerability where relayers can drain asset pools of all ERC-20 tokens and ETH, we recommend to make sure that in calculateFee, the amount[i] > serviceCharge[i] + relayerRefund[i] check is carried out in all cases, including when amount[i] is zero. This can be done, for example, with the following change:

function calculateFee(
    uint256[4] calldata amount,
    uint256[4] calldata relayerRefund
)
    external
    view
    returns (uint256[4] memory, uint256[4] memory, uint256[4] memory)
{
    uint256 serviceFeePercent = _serviceFeePercent;
    uint256[4] memory serviceCharge;
    uint256[4] memory actualAmount;
    for (uint256 i = 0; i < 4; i++) {
-         if (amount[i] != 0) {
            serviceCharge[i] = (amount[i] * serviceFeePercent) / _PRECISION;
            require(
                amount[i] > serviceCharge[i] + relayerRefund[i],
                "FeeManager: amount must be greater than fees"
            );
            actualAmount[i] =
                amount[i] -
                serviceCharge[i] -
                relayerRefund[i];
-         }
    }

    return (actualAmount, serviceCharge, relayerRefund);
}

Remediation

This issue has been acknowledged by Singularity, and a fix was implemented in commit f9784a0c↗.