Rounding error in fee comparison logic resulting in denial of service.

Summary

Differences in rounding direction between fee generation logic and fee validation logic lead to denial of service for token amounts containing high-precision decimals.

Vulnerability Detail

When a ResolvedOrder is prepared for execution, it is injected with fees via a call to _injectFees(ResolvedOrder), where the following sanity check is performed:

if (feeOutput.amount > tokenValue.mulDivDown(MAX_FEE, DENOM)) {
    revert FeeTooLarge(
        feeOutput.token,
        feeOutput.amount,
        feeOutput.recipient
    );
}

This helps ensure that an intent can never be charged more than the protocol's MAX_FEE bps.

Notice that during this comparison, we use Solmate's mulDivDown(uint256,uint256,uint256) to calculate the upper threshold on the maximum possible fee for the given token quantity by rounding down the aggregate output amount. If the computed feeOutput.amount is greater than this value, the order will not be permitted to be executed.

Conversely, when initially calculate the feeOuptut amounts in a call to RubiconFeeController's getFeeOutputs(ResolvedOrder), we are instead rounding up using mulDivUp(uint256,uint256,uint256):

uint256 feeAmount = fee.applyFee
    ? order.outputs[i].amount.mulDivUp(fee.fee, DENOM) /* @audit i.e. order.outputs[i].amount.mulDivUp(MAX_FEE, DENOM) */
    : order.outputs[i].amount.mulDivUp(baseFee, DENOM) /* @audit i.e. order.outputs[i].amount.mulDivUp(MAX_FEE, DENOM) */;

If we assume either a protocol baseFee or PairBasedFee configured to the protocol's MAX_FEE, it is clear that the rounding differences for lower-order fee bits will cause the execution of compliant orders to revert with FeeTooLarge(address,uint256,address), as the computed output fee amount will be naturally larger due to the difference in choice of rounding direction.

This has the effect of preventing valid orders from being fulfilled when operating at the MAX_FEE for a given base fee or pair.

To ground this finding in reality, it should be emphasised that the protocol expressly states that it is intentional to grant authorized address to specify protocol fees at the MAX_FEE (inclusive):

function setBaseFee(uint256 bFee) external auth {
    if (bFee == 0 || bFee > gladiusReactor.MAX_FEE())
        revert InvalidFee();
    baseFee = bFee;
}

Impact

The ability for an authorized actor to configure fees to the protocol maximum is well within the bounds of expected operation of the protocol, and if done so, will result in the denial of service for any valid transactions in possession of non-zero high precision decimals.

Although most manual user input amounts will not specify high-precision decimals, this assumption does not hold true for programmatically calculated intent amounts, such as those computed as the result of a swap, or those selected an effort to secure an intentionally precise output value.

This affects token input amounts of all size, and is not an issue that relates to only dust amounts.

For demonstration, let's consider the token amount 1.000000000000009999 ether:

➜ mulDivDown(1000000000000009999, 1_000, 100_000)
Type: uint256
├ Hex: 0x000000000000000000000000000000000000000000000000002386f26fc10063
├ Hex (full word): 0x000000000000000000000000000000000000000000000000002386f26fc10063
└ Decimal: 10000000000000099

mulDivUp(1000000000000009999, 1_000, 100_000)
Type: uint256
├ Hex: 0x000000000000000000000000000000000000000000000000002386f26fc10064
├ Hex (full word): 0x000000000000000000000000000000000000000000000000002386f26fc10064
└ Decimal: 10000000000000100

As we can see, even when base token amounts are very large in comparison to the dust decimal amounts, they mere presence of dust amounts in high-precision decimals is enough to to trigger rounding comparison differences between _injectFees(ResolvedOrder) and getFeeOutputs(ResolvedOrder).

Although this can indeed lead to a significant denial of service for generated intents, it only occurs when the fee settings are configured to their (near-)maximum, reducing their likelihood. In this case I am inclined to assess this issue as medium severity.

Code Snippet

/// @inheritdoc IProtocolFeeController
/// @notice Applies fee on output values in the form of output[0].token.
function getFeeOutputs(
    ResolvedOrder memory order
) external view override returns (OutputToken[] memory result) {    
    /// @notice Right now the length is enforced by
    ///         'GladiusReactor' to be equal to 1.
    result = new OutputToken[](order.outputs.length);

    address tokenIn = address(order.input.token);
    uint256 feeCount;

    for (uint256 i = 0; i < order.outputs.length; ++i) {
        /// @dev Fee will be in the 'tokenOut' form.
        address tokenOut = order.outputs[i].token;

        PairBasedFee memory fee = fees[
            getPairHash(address(tokenIn), tokenOut)
        ];

        uint256 feeAmount = fee.applyFee
            ? order.outputs[i].amount.mulDivUp(fee.fee, DENOM)
            : order.outputs[i].amount.mulDivUp(baseFee, DENOM);

        /// @dev If fee is applied to pair.
        if (feeAmount != 0) {
            bool found;

            for (uint256 j = 0; j < feeCount; ++j) {
                OutputToken memory feeOutput = result[j];

                if (feeOutput.token == tokenOut) {
                    found = true;
                    feeOutput.amount += feeAmount;
                }
            }

            if (!found) {
                result[feeCount] = OutputToken({
                    token: tokenOut,
                    amount: feeAmount,
                    recipient: feeRecipient
                });
                feeCount++;
            }
        }
    }

    assembly {
        // update array size to the actual number of unique fee outputs pairs
        // since the array was initialized with an upper bound of the total number of outputs
        // note: this leaves a few unused memory slots, but free memory pointer
        // still points to the next fresh piece of memory
        mstore(result, feeCount)
    }
}

Tool used

Foundry, Chisel

Recommendation

Round down when computing protocol fees in the RubiconFeeController to ensure consistency with the logic in ProtocolFees.

RubiconFeeController.sol

uint256 feeAmount = fee.applyFee
-    ? order.outputs[i].amount.mulDivUp(fee.fee, DENOM)
+    ? order.outputs[i].amount.mulDivDown(fee.fee, DENOM)
-    : order.outputs[i].amount.mulDivUp(baseFee, DENOM);
+    : order.outputs[i].amount.mulDivDown(baseFee, DENOM);

Duplicate of #51

sherlock-audit / 2024-02-rubicon-finance-judging

cawfree - Rounding error in fee comparison logic resulting in denial of service. #1