Dobry - `getFeeOutputs` does not work as intended due to a wrong comparison

[sherlock-admin2]

Dobry

medium

getFeeOutputs does not work as intended due to a wrong comparison

medium

Summary

The getFeeOutputs does not work as intended due to a wrong comparison in the for loop.

Vulnerability Detail

In the function getFeeOutputs, the feeOutput.token is being compared to the result[j] in the second for loop. The loop iterates while j < feeCount. The problem here is that feeCount's default value is 0 and it is being updated at the end of the first for loop. This means that on the first iteration of the main loop, the feeCount's value would not have incremented to 1, which will lead to the for loop not entering its body, therefore skipping the check and missing the 0 index element.

Impact

The 0 index element will be skipped which will lead to wrong functionality of the function.

Code Snippet

getFeeOutputs function:

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

Tool used

Manual Review

Recommendation

In order to ensure that you cover the 0 index check, either add an additional check in the for loop for the case when feeCount equals 0 , or add an additional check for that specifical case outside of the for loop.

[sherlock-admin]

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

tsvetanovv commented:

I think the function works as it should and and the loop should be skipped the first time

0xAadi commented:

Invalid: wrong statement