search

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

5 stars 3 forks source link

pkqs90 - When `applyFee` is set to false, the base fee continues to be applied rather than being disabled. #15

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

pkqs90

medium

When applyFee is set to false, the base fee continues to be applied rather than being disabled.

Summary

When applyFee is set to false, the base fee continues to be applied rather than being disabled.

Vulnerability Detail

The RubiconFeeController is responsible for handling fees. There are two fee types: a pair-based dynamic fee and a global base fee. The code comments indicates that disabling applyFee should turn off both the dynamic and base fees. However, the base fee continues to be applied.

Impact

The base fee is mistakenly applied even when it should be disabled, resulting in unnecessary fees being charged on trades.

Code Snippet

RubiconFeeController.sol

    /// @dev Fee controller, that's intended to be called by reactors.
    ///      * By default applies constant 'BASE_FEE' on output token.
    ///      * Dynamic pair-based fee can be enabled by calling 'setPairBasedFee'.
>   ///      * Both dynamic and base fee can be disabled by setting 'applyFee' to false.

RubiconFeeController.sol

        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;

Tool used

VSCode

Recommendation

Currently, there is no mechanism to disable the base fee. It is recommended to introduce a global flag to manage the base fee. An example implementation could be as follows:


bool applyBaseFee; // Global variable.

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

if (fee.applyFee) {
    feeAmount = order.outputs[i].amount.mulDivUp(fee.fee, DENOM);
} else if (applyBaseFee) {
    feeAmount = order.outputs[i].amount.mulDivUp(baseFee, DENOM)
} else {
    feeAmount = 0;
}

Duplicate of #22

sherlock-admin commented 7 months ago

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

0xAadi commented:

Invalid: The implementation is accurate, although the "@dev" comment appears to be a mistake.