WangAudit - [H] `RubiconFeeController::getFeeOutputs` incorrectly creates feeOutput tokens, thus adding duplicates to the `order.outputs`

[sherlock-admin2]

WangAudit

medium

[H] RubiconFeeController::getFeeOutputs incorrectly creates feeOutput tokens, thus adding duplicates to the order.outputs

high

Summary

The problem is that order.outputs and feeOutputs arrays contain the same tokens, and in the end we add feeOutputs to the order.outputs and pay out the same output twice.

Vulnerability Detail

As we can see, there is a check for the tokens with a special fee if (feeAmount != 0). Then it starts looping through the results arrays with the condition j = 0; j < feeCount; j++). The first interaction will be skipped cause feeCount is zero. Therefore, bool found will stay false and enter if (!found) clause. And we add current tokenOut to the result array.

Code for the text above:

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

Then we return results array to the ProtocolFees::_injectFees function. I know this contract is out of scope, but BaseGladiusReactor inherits from it and implements _injectFees function. At some point at L75 there is a check that feeOutput token is in outputs array. But the function only increments token's value after it. And in the end of the _injectFees function we add feeOutput token to the newOutputs array. newOutputs array is created at L50 and contains all the tokens from order.outputs (in this implementation only one token). Since feeOutput is the same token, now newOutputs contains the same token twice.

Snippet from `_injectFees`

```solidity function _injectFees(ResolvedOrder memory order) internal view { if (address(feeController) == address(0)) { return; } OutputToken[] memory feeOutputs = feeController.getFeeOutputs(order); uint256 outputsLength = order.outputs.length; uint256 feeOutputsLength = feeOutputs.length; // apply fee outputs // fill new outputs with old outputs OutputToken[] memory newOutputs = new OutputToken[]( outputsLength + feeOutputsLength ); unchecked { for (uint256 i = 0; i < outputsLength; i++) { newOutputs[i] = order.outputs[i]; } } for (uint256 i = 0; i < feeOutputsLength; ) { OutputToken memory feeOutput = feeOutputs[i]; // assert no duplicates unchecked { for (uint256 j = 0; j < i; j++) { if (feeOutput.token == feeOutputs[j].token) { revert DuplicateFeeOutput(feeOutput.token); } } } // assert not greater than MAX_FEE uint256 tokenValue; for (uint256 j = 0; j < outputsLength; ) { OutputToken memory output = order.outputs[j]; if (output.token == feeOutput.token) { tokenValue += output.amount; } unchecked { j++; } } // allow fee on input token as well if (address(order.input.token) == feeOutput.token) { tokenValue += order.input.amount; } if (tokenValue == 0) revert InvalidFeeToken(feeOutput.token); if (feeOutput.amount > tokenValue.mulDivDown(MAX_FEE, DENOM)) { revert FeeTooLarge( feeOutput.token, feeOutput.amount, feeOutput.recipient ); } newOutputs[outputsLength + i] = feeOutput; unchecked { i++; } } order.outputs = newOutputs; } ```

As we continue to go through the function till we reach the transfer of the output tokens inside transferFill function, there are no checks to if there are duplicates inside order.outputs and protocol ends up sending the same token amount twice.

Impact

Contract loses money, cause it sends out output tokens twice for the swap.

Code Snippet

RubiconFeeController::getFeeOutputs ProtocolFees::_injectFees BaseGladiusReactor::_fill

Tool used

Manual Review

Recommendation

In any of the functions add a check that feeOutput token is inside the outputs array, and if yes, then change the amount for that particular token.

[sherlock-admin]

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

0xAadi commented:

Invalid: wrong staement, even if the tokens in the order.outputs are same, but its receipient will be different, fee token will goes to fee recipient address

[daoio]

it's an intended behavior, there are no limitations on token duplicates and note that the fee should be paid exactly to the feeRecipient, whose address is set as a recipient in this new element in the outputs array.