Trader Joe v2 contest - hyh's results

swapAVAXForExactTokens() logic includes transferring out the excess amount of the native funds supplied by a caller.

However, amountsIn[0] - msg.value amount that swapAVAXForExactTokens() calculates for transfer out is negative. The reason is the inverted amount calculation, i.e. according to the logic in this case msg.value - amountsIn[0] to be used instead. This way the initial call will revert whenever there is no exact match, i.e. all the calls with amountsIn[0] <> msg.value will be reverted. As DEXes are being heavily used by downstream systems such denial of service can lead to failures snowballing with the corresponding monetary losses for the users of those client protocols. As a matter of example funds being permanently stuck in a bridge as swapping of this type is a mandatory operation before withdrawal. Similar withdrawal blocking situations can occur for the various Vaults, and so forth. Notice that entries there involve swapping of the another kind, so it will be a frozen funds scenario instead of denial of service for such systems.

Setting the severity to be high as that's core functionality unavailability whenever native token value incoming is greater than needed, which happens almost always as the amountsIn[0] needed is conditional on the current state of the system and is calculated on the fly, i.e. the only mainstream use case for the system is supplying excess native funds. An alternative is to calculate it beforehand and run a call with exact msg.value and a bloated gas cost to swap before Pair's state changes. This is MEV usage scenario, which represents a tiny share of overall native to exact tokens swapping.

Proof of Concept

swapAVAXForExactTokens() tries to transfer amountsIn[0] - msg.value when msg.value > amountsIn[0], i.e. when amountsIn[0] - msg.value < 0:

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L520-L521

        if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value);
    }

Such call of _safeTransferAVAX(_to, negative_amount) will revert as uint256 _amount is expected:

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L960-L966

    /// @notice Helper function to transfer AVAX
    /// @param _to The address of the recipient
    /// @param _amount The AVAX amount to send
    function _safeTransferAVAX(address _to, uint256 _amount) private {
        (bool success, ) = _to.call{value: _amount}("");
        if (!success) revert LBRouter__FailedToSendAVAX(_to, _amount);
    }

This way any swapAVAXForExactTokens() call with msg.value > amountsIn[0] will revert.

As amountsIn are system estimated this means that the vast majority of swapAVAXForExactTokens() calls will be reverted.

I.e. as native token deficit imply expected swapAVAXForExactTokens() revert, being a part of system workflow, basically all usages of the function will have excess native funds supplied:

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L509-L512

        address[] memory _pairs = _getPairs(_pairBinSteps, _tokenPath);
        amountsIn = _getAmountsIn(_pairBinSteps, _pairs, _tokenPath, _amountOut);

        if (amountsIn[0] > msg.value) revert LBRouter__MaxAmountInExceeded(msg.value, amountsIn[0]);

That means that all calls to swapAVAXForExactTokens() with amountsIn[0] <> msg.value be reverted, native to exact tokens swapping is basically unavailable.

Recommended Mitigation Steps

The msg.value > amountsIn[0] excess value check is correct, while the amount calculation needs to be inverted:

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L520-L521

-       if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value);
+       if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, msg.value - amountsIn[0]);
    }