Router Manual Review Findings

RRE-01M: Overly Strict Evaluation of Output

Type	Severity	Location
Mathematical Operations		Router.sol:L241

Description:

The exactOutputInternal function will fail if the amountOutReceived does not match the amountOut value exactly which can be a side-effect of multiple language-specific issues such as mathematical truncation.

Impact:

As the pricing modules of each IPool are unknown, we cannot assess whether an edge condition whereby the output does not match the expected one can manifest.

Example:

Copy
241require(amountOutReceived == amountOut, "amountOutReceived does not match amountOut in exactOutputInternal");

Recommendation:

We advise the conditional to ensure that the amountOutReceived is greater-than-or-equal-to (>=) the value of amountOUt instead, ensuring that the output can also exceed the expected one.

Alleviation:

The exactOutputInternal function has been commented out of the codebase rendering this exhibit no longer applicable.

RRE-02M: Incorrect Payable Function Attributes

Type	Severity	Location
Logical Fault		Router.sol:L77, L97-L98, L104, L142-L143, L149, L162-L163, L170, L182-L183

Description:

The referenced functions are set as payable and contain a specialized refund methodology at their end whilst they are not meant to handle native funds.

Impact:

The current implementations are redundantly complex and should not handle native funds which are refunded to the msg.sender instead of the intended recipient of the swap.

Example:

Copy
75function exactInputSingle(
76    ExactInputSingleParams memory params
77) external payable override nonReentrant returns (uint256 amountOut) {
78    require(tx.origin == msg.sender, "only EOA");
79    // // use amountIn == Constants.CONTRACT_BALANCE as a flag to swap the entire balance of the contract
80    bool hasAlreadyPaid;
81    (Orders.Order memory order,) = params.orders.decodeFirstOrder();
82    if (params.amountIn == 0) { //Constants.CONTRACT_BALANCE) {
83        hasAlreadyPaid = true;
84        //(Orders.Order memory order, ) = params.orders.decodeFirstOrder();
85        params.amountIn = IERC20(order.sellerToken).balanceOf(address(this));
86    }
87    require(order.seller == msg.sender, "seller is not correct");
88    emit swapCalculations(params.amountIn, params.recipient);
89
90    amountOut = exactInputInternal(
91        params.amountIn,
92        params.recipient,
93        SwapCallbackData({orders: params.orders, payer: hasAlreadyPaid ? address(this) : msg.sender})
94    );
95    require(amountOut >= params.amountOutMinimum, "Too little received");
96
97    if (address(this).balance > 0) 
98        TransferHelper.safeTransferETH(msg.sender, address(this).balance);
99}

Recommendation:

We advise the specialized refund methodology to be omitted and the payable attribute each one boasts to be removed.

Alleviation:

After extensive discussions with the Native team, we concluded that our initial assumption about the system's exchange mechanisms pertaining native assets was incorrect. As such, this finding is invalid.

RRE-03M: Inexistent Protection Against Multi-Order Invocation

Type	Severity	Location
Logical Fault		Router.sol:L147-L165

Description:

The exactOutputSingle will misbehave if it is invoked with a params.orders argument that contains more than one orders.

Impact:

If a user invokes the wrong function, they will end up evaluating an incorrect slippage value for the amountInMaximum comparison thus causing them to incur slippage they cannot protect against.

Example:

Copy
146/// @inheritdoc ISwapRouter
147function exactOutputSingle(
148    ExactOutputSingleParams calldata params
149) external payable override nonReentrant returns (uint256 amountIn) {
150    require(tx.origin == msg.sender, "only EOA");
151    // avoid an SLOAD by using the swap return data
152    amountIn = exactOutputInternal(
153        params.amountOut,
154        params.recipient,
155        SwapCallbackData({orders: params.orders, payer: msg.sender})
156    );
157
158    require(amountIn <= params.amountInMaximum, "Too much requested");
159    // has to be reset even though we don't use it in the single hop case
160    amountInCached = DEFAULT_AMOUNT_IN_CACHED;
161
162    if (address(this).balance > 0) 
163        TransferHelper.safeTransferETH(msg.sender, address(this).balance);
164
165}
166
167/// @inheritdoc ISwapRouter
168function exactOutput(
169    ExactOutputParams calldata params
170) external payable override nonReentrant returns (uint256 amountIn) {
171    require(tx.origin == msg.sender, "only EOA");
172    exactOutputInternal(
173        params.amountOut,
174        params.recipient,
175        SwapCallbackData({orders: params.orders, payer: msg.sender})
176    );
177
178    amountIn = amountInCached;
179    require(amountIn <= params.amountInMaximum, "Too much requested");
180    amountInCached = DEFAULT_AMOUNT_IN_CACHED;
181
182    if (address(this).balance > 0) 
183        TransferHelper.safeTransferETH(msg.sender, address(this).balance);
184
185}

Recommendation:

We advise it to properly ensure only one order is contained in the params.orders encoded byte structure by validating its length.

Alleviation:

The exactOutputSingle function has been commented out of the codebase, however, the exactInputSingle function now properly applies a check to ensure that the params.orders member does not have multiple pools declared. As the exhibit only pertained to the exactOutputSingle function, we consider this exhibit nullified.

RRE-04M: Inoperable Output Swap Mechanism

Type	Severity	Location
Logical Fault		Router.sol:L65, L168-L185

Description:

The exactOutput function behaves in a significantly atypical manner in that it expects the input params.orders to be in reverse with the final output in the first slot and the original input in the last slot. Additionally, it performs a deliberate re-entrancy during the swapCallback hook which in turn can easily run out of the limited stack space available on the EVM.

Impact:

The current exactOutput mechanism is unsafe and inoperable in production due to the significant gas cost as well as stack space it utilizes, causing it to frequently run out of memory (OOM), out of gas (OOG), and / or out of stack space (OOS). Additionally, if a pool is entered twice as part of a multi-order swap the transaction will fail as each pool has a nonReentrant protection mechanism in its swap function.

Example:

Copy
167/// @inheritdoc ISwapRouter
168function exactOutput(
169    ExactOutputParams calldata params
170) external payable override nonReentrant returns (uint256 amountIn) {
171    require(tx.origin == msg.sender, "only EOA");
172    exactOutputInternal(
173        params.amountOut,
174        params.recipient,
175        SwapCallbackData({orders: params.orders, payer: msg.sender})
176    );
177
178    amountIn = amountInCached;
179    require(amountIn <= params.amountInMaximum, "Too much requested");
180    amountInCached = DEFAULT_AMOUNT_IN_CACHED;
181
182    if (address(this).balance > 0) 
183        TransferHelper.safeTransferETH(msg.sender, address(this).balance);
184
185}

Recommendation:

As recursion as well as deliberate re-entrancies are strongly advised against, we advise both traits to be removed from the codebase by revamping the way exact output trades happen. This can be achieved by using a similar paradigm to Uniswap V2 whereby they measure the amountIn prior to executing the trades rather than dynamically performing them via recursion.

Alleviation:

The exactOutputSingle and exactOutput functions have been commented out of the codebase rendering this exhibit no longer applicable.