Trader Joe v2 contest - rbserver's results

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBRouter.sol#L485-L521

Vulnerability details

Impact

When calling the swapAVAXForExactTokens function, if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value) is executed, which is for refunding any excess amount sent in; this is confirmed by this function's comment as well. However, executing amountsIn[0] - msg.value will always revert when msg.value > amountsIn[0] is true. Developers who has the design of the swapAVAXForExactTokens function in mind could develop front-ends and contracts that will send excess amount when calling the swapAVAXForExactTokens function. Hence, the users, who rely on these front-ends and contracts for interacting with the swapAVAXForExactTokens function will always find such interactions being failed since calling this function with the excess amount will always revert. As a result, the user experience becomes degraded, and the usability of the protocol becomes limited.

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBRouter.sol#L485-L521

    /// @notice Swaps AVAX for exact tokens while performing safety checks
    /// @dev will refund any excess sent
    ...
    function swapAVAXForExactTokens(
        uint256 _amountOut,
        uint256[] memory _pairBinSteps,
        IERC20[] memory _tokenPath,
        address _to,
        uint256 _deadline
    )
        external
        payable
        override
        ensure(_deadline)
        verifyInputs(_pairBinSteps, _tokenPath)
        returns (uint256[] memory amountsIn)
    {
        ...

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

Proof of Concept

Please add the following test in test\LBRouter.Swaps.t.sol. This test will pass to demonstrate the described scenario.

    function testSwapAVAXForExactTokensIsUnableToRefund() public {
        uint256 amountOut = 1e18;

        (uint256 amountIn, ) = router.getSwapIn(pairWavax, amountOut, false);

        IERC20[] memory tokenList = new IERC20[](2);
        tokenList[0] = wavax;
        tokenList[1] = token6D;
        uint256[] memory pairVersions = new uint256[](1);
        pairVersions[0] = DEFAULT_BIN_STEP;

        vm.deal(DEV, amountIn + 500);

        // Although the swapAVAXForExactTokens function supposes to refund any excess sent,
        //   calling it reverts when sending more than amountIn
        //   because executing _safeTransferAVAX(_to, amountsIn[0] - msg.value) results in arithmetic underflow
        vm.expectRevert(stdError.arithmeticError);
        router.swapAVAXForExactTokens{value: amountIn + 1}(amountOut, pairVersions, tokenList, DEV, block.timestamp);
    }

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBRouter.sol#L520 can be updated to the following code.

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

function testSwapAVAXForExactTokensIsUnableToRefund() public {
        uint256 amountOut = 1e18;

        (uint256 amountIn, ) = router.getSwapIn(pairWavax, amountOut, false);

        IERC20[] memory tokenList = new IERC20[](2);
        tokenList[0] = wavax;
        tokenList[1] = token6D;
        uint256[] memory pairVersions = new uint256[](1);
        pairVersions[0] = DEFAULT_BIN_STEP;

        vm.deal(DEV, 50 * amountIn + 500);


        // Although the swapAVAXForExactTokens function supposes to refund any excess sent,
        //   calling it reverts when sending more than amountIn
        //   because executing _safeTransferAVAX(_to, amountsIn[0] - msg.value) results in arithmetic underflow
        vm.expectRevert(stdError.arithmeticError);
        router.swapAVAXForExactTokens{value: amountIn + 1}(amountOut, pairVersions, tokenList, DEV, block.timestamp);

        router.swapAVAXForExactTokens{value: amountIn}(amountOut, pairVersions, tokenList, DEV, block.timestamp);

        vm.expectRevert(stdError.arithmeticError);
        router.swapAVAXForExactTokens{value: amountIn + 100000}(amountOut, pairVersions, tokenList, DEV, block.timestamp);


    }

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBPair.sol#L304-L413 https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/TokenHelper.sol#L40-L52 https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/TokenHelper.sol#L74-L81

Vulnerability details

Impact

When calling the swap function, tokenY.safeTransfer(_to, _amountOut) or tokenX.safeTransfer(_to, _amountOut) is executed, where the TokenHelper library's safeTransfer function is used. According to the implementation of the TokenHelper library's safeTransfer function, tokenY and tokenX's contract existences are not checked. It is possible that a token becomes non-existent in the future; for instance, if some bugs are found for the output token, it is possible that the output token will be destroyed through calling its selfdestruct function after migrating its data to another contract for fixing these bugs. When this happens, such output token becomes non-existent but calling the swap function to swap for it does not revert. As a result, the user who swaps for a non-existent output token will lose the input token amount while receiving no output token amount.

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBPair.sol#L304-L413

    function swap(bool _swapForY, address _to)
        external
        override
        nonReentrant
        returns (uint256 amountXOut, uint256 amountYOut)
    {
        ...

        if (_swapForY) {
            amountYOut = _amountOut;
            tokenY.safeTransfer(_to, _amountOut);
        } else {
            amountXOut = _amountOut;
            tokenX.safeTransfer(_to, _amountOut);
        }
    }

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/TokenHelper.sol#L40-L52

    function safeTransfer(
        IERC20 token,
        address recipient,
        uint256 amount
    ) internal {
        if (amount != 0) {
            (bool success, bytes memory result) = address(token).call(
                abi.encodeWithSelector(token.transfer.selector, recipient, amount)
            );

            _catchTransferError(success, result);
        }
    }

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/TokenHelper.sol#L74-L81

    function _catchTransferError(bool success, bytes memory result) private pure {
        // Look for revert reason and bubble it up if present
        if (!(success && (result.length == 0 || abi.decode(result, (bool))))) {
            assembly {
                revert(add(32, result), mload(result))
            }
        }
    }

Proof of Concept

First, in test\mocks\, please add the following mock token contract.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import './ERC20.sol';

contract ERC20MockWithSelfdestruct is ERC20Mock {
  constructor() ERC20Mock("Test", "T", 18) {}

  function kill() external {
    selfdestruct(payable(msg.sender));
  }
}

Then, please add the following code in test\LBPair.Swaps.t.sol.

1. Import ERC20MockWithSelfdestruct as follows.

import "./mocks/ERC20MockWithSelfdestruct.sol";

2. Add the following state variables in the LiquidityBinPairSwapsTest contract.

    LBPair pair_;
    ERC20MockWithSelfdestruct erc20MockWithSelfdestruct;
    uint256 amountXIn_;

3. Add the following code for POC purpose in the setUp function. Note this will break other tests so please only use this setUp code for this test.

    function setUp() public {
        token6D = new ERC20MockDecimals(6);
        token18D = new ERC20MockDecimals(18);

        factory = new LBFactory(DEV, 8e14);
        ILBPair _LBPairImplementation = new LBPair(factory);
        factory.setLBPairImplementation(address(_LBPairImplementation));
        setDefaultFactoryPresets(DEFAULT_BIN_STEP);
        addAllAssetsToQuoteWhitelist(factory);
        router = new LBRouter(ILBFactory(DEV), IJoeFactory(DEV), IWAVAX(DEV));

        pair = createLBPairDefaultFees(token6D, token18D);

        /** add the following code for POC purpose */

        // deploy the ERC20 mock token, which has the selfdestruct functionality
        erc20MockWithSelfdestruct = new ERC20MockWithSelfdestruct();

        factory.addQuoteAsset(erc20MockWithSelfdestruct);

        // create a pair with token6D and erc20MockWithSelfdestruct
        pair_ = createLBPairDefaultFees(token6D, erc20MockWithSelfdestruct);

        uint256 tokenAmount = 100e18;
        erc20MockWithSelfdestruct.mint(address(pair_), tokenAmount);

        uint256[] memory _ids = new uint256[](1);
        _ids[0] = ID_ONE;

        uint256[] memory _liquidities = new uint256[](1);
        _liquidities[0] = Constants.PRECISION;

        pair_.mint(_ids, new uint256[](1), _liquidities, DEV);

        amountXIn_ = 10e18;

        (uint256 amountYOut_, ) = router.getSwapOut(pair_, amountXIn_, true);

        // ALICE has amountXIn_*2 of token6D at this moment
        token6D.mint(ALICE, amountXIn_ * 2);

        // ALICE swaps amountXIn_ of token6D for erc20MockWithSelfdestruct
        vm.startPrank(ALICE);
        token6D.transfer(address(pair_), amountXIn_);
        pair_.swap(true, ALICE);
        vm.stopPrank();

        // after this swap, ALICE has amountXIn_ of token6D left and has received amountYOut_ of erc20MockWithSelfdestruct
        assertEq(token6D.balanceOf(ALICE), amountXIn_);
        assertEq(erc20MockWithSelfdestruct.balanceOf(ALICE), amountYOut_);
        
        // simulate a situation where mockERC20WithSelfdestruct is destroyed after its data is migrated to another contract for fixing some bugs
        erc20MockWithSelfdestruct.kill();

        /** */
    }

4. Add the following test. This test will pass to demonstrate the described scenario.

    function testLoseInputTokenAndNotReceiveOutputTokenWhenOutputTokenBecomesNonExistent() public {
        // ALICE swaps amountXIn_ of token6D for erc20MockWithSelfdestruct again
        vm.startPrank(ALICE);
        token6D.transfer(address(pair_), amountXIn_);
        pair_.swap(true, ALICE);
        vm.stopPrank();

        // after ALICE's second swap, ALICE has no token6D left
        assertEq(token6D.balanceOf(ALICE), 0);

        // however, ALICE has not received any erc20MockWithSelfdestruct in return this time since erc20MockWithSelfdestruct does not exist anymore
        vm.expectRevert(bytes(""));
        erc20MockWithSelfdestruct.balanceOf(ALICE);
    }

Tools Used

VSCode

Recommended Mitigation Steps

In the TokenHelper library's safeTransfer function, the existence of the contract for token should be checked before performing the low-level call. If such contract does not exist, calling this function should revert.