search

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

5 stars 3 forks source link

pkqs90 - Contract upgrades may cause DoS due to `MAX_FEE` constraint #16

Closed sherlock-admin closed 9 months ago

sherlock-admin commented 9 months ago

pkqs90

medium

Contract upgrades may cause DoS due to MAX_FEE constraint

Summary

The GladiusReactor and RubiconFeeController are two separate contracts capable of independent upgrades. The admin of the RubiconFeeController has the authority to adjust fees, subject to the condition that they do not exceed the MAX_FEE threshold specified in the GladiusReactor. However, due to the possibility of independent upgrades to each contract, this limitation might not be enforced following such updates. This discrepancy could result in a DoS during the execution of trades, specifically within the _injectFees function.

Vulnerability Detail

The RubiconFeeController contract manages two types of fees: pair-wise dynamic fees and a global base fee, both of which are determined by the RubiconFeeController admin and must not exceed the MAX_FEE limit defined by gladiusReactor.MAX_FEE(). GladiusReactor, on its part, applies fees based on the outputs from RubiconFeeController, and will revert transactions if the fees exceed the MAX_FEE restriction. At first glance, this setup may appear to work seamlessly. However, considering that both contracts are upgradeable, the constraints between the two contracts could diverge following an upgrade. This divergence could lead to a DoS during the fee injection phase of trading operations.

Impact

Following a contract upgrade, the fees set in RubiconFeeController might exceed GladiusReactor's MAX_FEE threshold, potentially causing a DoS for trade operations.

Code Snippet

ProtocolFees.sol

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

RubiconFeeController.sol

    function setPairBasedFee(
        address tokenIn,
        address tokenOut,
        uint256 fee,
        bool applyFee
    ) external auth {
>   if (fee > gladiusReactor.MAX_FEE())
        revert InvalidFee();
        bytes32 pairHash = getPairHash(tokenIn, tokenOut);
        fees[pairHash] = PairBasedFee({applyFee: applyFee, fee: fee});
    }

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

Tool used

VSCode

Recommendation

Rather than reverting when fees exceed the allowable limit, it's advised to collect only up to the protocol's MAX_FEE as fees and to emit an event. This event can then be detected by a monitoring system.

sherlock-admin commented 9 months ago

2 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

In my opinion this is a Low severity issue. Additionally it may count as an Admin mistake and somewhat the protocol is familiar with this - https://github.com/sherlock-audit/2024-02-rubicon-finance/tree/11cac67919e8a1303b3a3177291b88c0c70bf03b?tab=readme-ov-file#q-are-there-any-additional-protocol-roles-if-yes-please-explain-in-detail

0xAadi commented:

Invalid: Admin/Owner is Trusted and likeliness is very low