trauki - Medium - Flawed logic inside `getFeeOutputs()` function

[sherlock-admin]

trauki

medium

Medium - Flawed logic inside getFeeOutputs() function

Summary

getFeeOutputs() applies fees to the output token and returns it, but the logic inside this function is flawed.

Vulnerability Detail

First, this loop is supposed to update the fee amount if it is the correct output token, except under the current logic, this loop can never be reached. This is because feeCount is initialized with the default uint256 value of 0, never updated, and then used inside the for loop conditional that says "run while 0 is less than 0", which obviously can never be true.

uint256 feeCount;
for (uint256 j = 0; j < feeCount; ++j) {

Even if we updated the for loop's conditional to say "run while 0 is less than or equal to 0" (j <= feeCount;), the next issue lies directly inside of the loop:

result = new OutputToken[](order.outputs.length);

 for (uint256 j = 0; j <= feeCount; ++j) {
                    OutputToken memory feeOutput = result[j];
                    if (feeOutput.token == tokenOut) {
                        found = true;
                        feeOutput.amount += feeAmount;
                    }
                }

We initialize result as the empty OutputToken array with a length of 1, then we set feeOutput as an OutputToken object, using result, and finally, we try to compare a value between an empty object with uninitialized values, to an actual value that was passed as input to the function. This means it is essentially the same as this when using an actual ERC20 output:

if(address(0) == address(1337)

Unless using native currency like ETH as address(0), this if statement will never be true meaning the bool found will always be false, and feeOutput.amount will never be incremented by feeAmount. However, even if the previous logic was correct so that this if statement could be reached, the only thing returned from the function would be the empty result object since it is never actually updated.

There is one additional bug I found in this function: if the feeAmount is returned as 0, meaning there is no fee on the pair, then the result is returned as an empty OutputToken[](1) with default values for everything, which will once again cause any future logic relying on these variables to fail or break in unexpected ways.

if (feeAmount != 0) {

Impact

Code inside of getFeeOutputs() can never be reached or could return a struct with uninitialized values.

Code Snippet

This is the entire function that I broke apart and explained above, you can view the entire file here.

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

• Update the for loop to use a conditional that says j <= feeCount as opposed to <.
• Revise logic on how local variables are set so that uninitialized values aren't carried over to new variables (unless this is intentional).
• Ensure values that are used inside conditional if statements are initialized.
• Update the else conditionals or at least ensure that the result object that is returned never contains empty/default values.

[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, feeCount is updating in the loop