Filling orders may revert due to inconsistent fee rounding

Summary

Filling orders may revert for fee numerators close to MAX_FEE in ProtocolFees due to inconsistent rounding between fee calculation in RubiconFeeController and fee validation in ProtocolFees.

Vulnerability Detail

When orders are filled, fees added to an order when filled are calculated in ProtocolFees::_injectFees with a call to RubiconFeeController::getFeeOutputs. For each output config in the order, the fee is rounded up with the total fee for each unique output token aggregated. https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/11cac67919e8a1303b3a3177291b88c0c70bf03b/gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol#L81-L96

            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;
                    }
                }

However, the fee output is checked later against the allowed max fee proportion of the total output token value, calculated by rounding down. https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/11cac67919e8a1303b3a3177291b88c0c70bf03b/gladius-contracts-internal/src/base/ProtocolFees.sol#L90-L96

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

Consequently, if the fee for the applicable token pair (base or custom) is set close to MAX_FEE (1%), orders may not be fulfillable due to triggering the above revert consequent to the calculated output fee rounding above the calculated max fee.

When the fee for the applicable token pair is equal to MAX_FEE, this will apply to any order with an output amount that is not perfectly divisible by MAX_FEE / DENOM = 100. As a general trend, orders with output tokens with lower precision (lower decimals) are more likely to be affected by fee rates set below MAX_FEE due to greater precision loss. It should also be noted that if reactors supporting more than one output are implemented, for orders that have multiple outputs for the same token, the impact of rounding up during fee calculation will be amplified and will make it more likely for valid orders to revert due to exceeding the max fee.

The issue when the fee is set to MAX_FEE can be demonstrated by modifying the fuzz test version of BaseGladiusReactorTest::test_GladiusExecuteWithFee to have a constant uint256 feeBps = reactor.MAX_FEE() (uint8 doesn't contain MAX_FEE=1000) (the range of the output amount can also be changed) and running this test targeting GladiusReactorTest (which inherits BaseGladiusReactorTest::test_GladiusExecuteWithFee and implements necessary helper functions). The test should fail with an output amount that is not divisible by 100. The test may also be ran with fee rates slightly lower than MAX_FEE (around 50 less) using the original bounds for the output amount (>= 1e3) (though this may require more runs to find a counterexample).

Impact

Orders may not be able to be fulfilled (highly likely if fee rate is set to MAX_FEE).

Code Snippet

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/11cac67919e8a1303b3a3177291b88c0c70bf03b/gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol#L81-L96 https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/11cac67919e8a1303b3a3177291b88c0c70bf03b/gladius-contracts-internal/src/base/ProtocolFees.sol#L90-L96

Tool used

Manual Review, Foundry

Recommendation

Round down when calculating fees in RubiconFeeController::getFeeOutputs.

Duplicate of #51

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

Kow - Filling orders may revert due to inconsistent fee rounding #7