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

5 stars 3 forks source link

bareli - wrong implementation of "getFeeOutputs" #41

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

bareli

medium

wrong implementation of "getFeeOutputs"

Summary

in the implementation of " function getFeeOutputs( ResolvedOrder memory order ) external view override returns (OutputToken[] memory result) "

we are going through every output token and we are trying to identify an actual number of unique fee output pairs.as we can see below code.

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

result array consists of all different tokens. as we are adding different tokens. result array length is less than equal to the output array length. lets take a scenario for some address tokenOut = order.outputs[i].token; as this will go through the "for (uint256 j = 0; j < feeCount; ++j) " so as result is all unique we can use a break statement after the
if (feeOutput.token == tokenOut) { found = true; feeOutput.amount += feeAmount; break; as this will save us a lot of gases.

Vulnerability Detail

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

Impact

too much gas cost.

Code Snippet

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol#L94

Tool used

Manual Review

Recommendation

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

sherlock-admin commented 7 months ago

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

tsvetanovv commented:

Invalid. These types of issues are not valid in Sherlock. Apart from that try to write your report better next time because it is hard to understand.

0xAadi commented:

Invalid: OOS