Trader Joe v2 contest - Jeiwan's results

Lines of code

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

Vulnerability details

Impact

Output amount is calculated incorrectly for a Trader Joe V1 pool when swapping tokens across multiple pools and some of the pools in the chain are V1 ones. Calculated amounts will always be smaller than expected ones, which will always affect chained swaps that include V1 pools.

Proof of Concept

LBRouter is a high-level contract that serves as the main contract users will interact with. The contract implements a lot of security checks and helper functions that make usage of LBPair contracts easier and more user-friendly. Some examples of such functions:

• swapExactTokensForTokensSupportingFeeOnTransferTokens, which makes chained swaps (i.e. swaps between tokens that don't have a pair) of tokens implementing fee on transfer (i.e. there's fee reduced from every transferred amount);
• swapExactTokensForAVAXSupportingFeeOnTransferTokens, which is the variation of the above function which takes AVAX as the output token;
• swapExactAVAXForTokensSupportingFeeOnTransferTokens, which is the variation of the previous function which takes AVA as the input token.

Under the hood, these three functions call _swapSupportingFeeOnTransferTokens, which is the function that actually performs swaps. The function supports both Trader Joe V1 and V2 pools: when _binStep is 0 (which is never true in V2 pools), it's assumed that the current pool is a V1 one. For V1 pools, the function calculates output amounts based on pools' reserves and balances:

if (_binStep == 0) {
    (uint256 _reserve0, uint256 _reserve1, ) = IJoePair(_pair).getReserves();
    if (_token < _tokenNext) {
        uint256 _balance = _token.balanceOf(_pair);
        uint256 _amountOut = (_reserve1 * (_balance - _reserve0) * 997) / (_balance * 1_000);

        IJoePair(_pair).swap(0, _amountOut, _recipient, "");
    } else {
        uint256 _balance = _token.balanceOf(_pair);
        uint256 _amountOut = (_reserve0 * (_balance - _reserve1) * 997) / (_balance * 1_000);

        IJoePair(_pair).swap(_amountOut, 0, _recipient, "");
    }
} else {
    ILBPair(_pair).swap(_tokenNext == ILBPair(_pair).tokenY(), _recipient);
}

However, these calculations are incorrect. Here's the difference:

@@ -888,12 +888,14 @@ contract LBRouter is ILBRouter {
                     (uint256 _reserve0, uint256 _reserve1, ) = IJoePair(_pair).getReserves();
                     if (_token < _tokenNext) {
                         uint256 _balance = _token.balanceOf(_pair);
-                        uint256 _amountOut = (_reserve1 * (_balance - _reserve0) * 997) / (_balance * 1_000);
+                        uint256 amountInWithFee = (_balance - _reserve0) * 997;
+                        uint256 _amountOut = (_reserve1 * amountInWithFee) / (_reserve0 * 1_000 + amountInWithFee);

                         IJoePair(_pair).swap(0, _amountOut, _recipient, "");
                     } else {
                         uint256 _balance = _token.balanceOf(_pair);
-                        uint256 _amountOut = (_reserve0 * (_balance - _reserve1) * 997) / (_balance * 1_000);
+                        uint256 amountInWithFee = (_balance - _reserve1) * 997;
+                        uint256 _amountOut = (_reserve0 * amountInWithFee) / (_reserve1 * 1_000 + amountInWithFee);

                         IJoePair(_pair).swap(_amountOut, 0, _recipient, "");
                     }

These calculations are implemented correctly in JoeLibrary.getAmountOut, which is used in LBQuoter. Also it's used in Trader Joe V1 to calculate output amounts in similar functions:

• https://github.com/traderjoe-xyz/joe-core/blob/main/contracts/traderjoe/JoeRouter02.sol#L375

// test/audit/RouterMath2.t.sol
// SPDX-License-Identifier: UNLICENSED

pragma solidity ^0.8.7;

import "../TestHelper.sol";

import "../../src/LBRouter.sol";
import "../../src/interfaces/IJoePair.sol";

contract RouterMath2Test is TestHelper {
    IERC20 internal token;
    uint256 internal actualAmountOut;

    function setUp() public {
        token = new ERC20MockDecimals(18);
        ERC20MockDecimals(address(token)).mint(address(this), 100e18);

        router = new LBRouter(
            ILBFactory(address(0x00)),
            IJoeFactory(address(this)),
            IWAVAX(address(0x02))
        );
    }

    // Imitates V1 factory.
    function getPair(address, /*tokenX*/ address /*tokenY*/ ) public view returns (address) {
        return address(this);
    }

    // Imitates V1 pool.
    function getReserves() public pure returns (uint112, uint112, uint32) {
        return (1e18, 1e18, 0);
    }

    // Imitates V1 pool.
    function balanceOf(address /*acc*/) public pure returns (uint256) {
        return 0.0001e18;
    }

    // Imitates V1 pool.
    function swap(uint256 amount0, uint256 amount1, address to, bytes memory data) public {
        actualAmountOut = amount0 == 0 ? amount1 : amount0;
    }

    function testScenario() public {
        // Setting up a swap via one V1 pool.
        uint256[] memory steps = new uint256[](1);
        steps[0] = 0;

        IERC20[] memory path = new IERC20[](2);
        path[0] = IERC20(address(token));
        path[1] = IERC20(address(this));

        uint256 amountIn = 0.0001e18;

        token.approve(address(router), 1e18);
        router.swapExactTokensForTokensSupportingFeeOnTransferTokens(
            amountIn, 0, steps, path, address(this), block.timestamp + 1000
        );
        // This amount was calculated incorrectly.
        assertEq(actualAmountOut, 987030000000000000); // Equals to 989970211528238869 when fixed.


        address _pair = address(this);
        uint256 expectedAmountOut;

        // Reproduce the calculations using JoeLibrary.getAmountIn. This piece:
        // https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBRouter.sol#L888-L899
        (uint256 _reserve0, uint256 _reserve1, ) = IJoePair(_pair).getReserves();
        if (address(token) < address(this)) {
            uint256 _balance = token.balanceOf(_pair);
            expectedAmountOut = JoeLibrary.getAmountOut(_balance - _reserve0, _reserve0, _reserve1);
        } else {
            uint256 _balance = token.balanceOf(_pair);
            expectedAmountOut = JoeLibrary.getAmountOut(_balance - _reserve1, _reserve1, _reserve0);
        }

        // This is the correct amount.
        assertEq(expectedAmountOut, 989970211528238869);

        // The wrong amount is smaller than the expected one.
        assertEq(expectedAmountOut - actualAmountOut, 2940211528238869);
    }
}

Tools Used

Manual review.

Recommended Mitigation Steps

Consider using the JoeLibrary.getAmountOut function in the _swapSupportingFeeOnTransferTokens function of LBRouter when computing output amounts for V1 pools.

Lines of code

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

Vulnerability details

Impact

Input amount is calculated incorrectly for Trader Joe V1 pools when swapping tokens across multiple pools and some of the pools in the chain are V1 ones. Calculated amounts will always be bigger than expected ones, which will always affect chained swaps that include V1 pools. In some situations (when swap amount * 997 is grater than the related reserve of a pool), it'll revert due to an underflow.

Proof of Concept

LBRouter is a high-level contract that serves as the main contract users will interact with. The contract implements a lot of security checks and helper functions that make usage of LBPair contracts easier and more user-friendly. Some examples of such functions:

• swapTokensForExactTokens, which allows to make chained swap, i.e. swap tokens that don't have a common pair;
• swapTokensForExactAVAX is the variation of the previous function which takes AVAX as the output token;
• swapAVAXForExactTokens is the variation of the first function which takes AVAX as the input token.

These three functions have one similarity: they swap an unknown amount of input token for a known (exact) amount of output tokens. To do that, they need to traverse the path of tokens in the reversed order and pre-compute input amounts for each swap–this is done in the _getAmountsIn function. The function supports both Trader Joe V1 and V2 pools: when _binStep is 0 (which is never true in V2 pools), it's assumed that the current pool is a V1 one. The function also uses different math for V1 and V2 pools:

if (_binStep == 0) {
    (uint256 _reserveIn, uint256 _reserveOut, ) = IJoePair(_pair).getReserves();
    if (_token > _tokenPath[i]) {
        (_reserveIn, _reserveOut) = (_reserveOut, _reserveIn);
    }

    uint256 amountOut_ = amountsIn[i];
    // Legacy uniswap way of rounding
    amountsIn[i - 1] = (_reserveIn * amountOut_ * 1_000) / (_reserveOut - amountOut_ * 997) + 1;
} else {
    (amountsIn[i - 1], ) = getSwapIn(ILBPair(_pair), amountsIn[i], ILBPair(_pair).tokenX() == _token);
}

However, the input amount calculation for V1 pools is incorrect. Here's the difference:

@@ -722,7 +722,7 @@ contract LBRouter is ILBRouter {

                 uint256 amountOut_ = amountsIn[i];
                 // Legacy uniswap way of rounding
-                amountsIn[i - 1] = (_reserveIn * amountOut_ * 1_000) / (_reserveOut - amountOut_ * 997) + 1;
+                amountsIn[i - 1] = (_reserveIn * amountOut_ * 1_000) / ((_reserveOut - amountOut_) * 997) + 1;
             } else {
                 (amountsIn[i - 1], ) = getSwapIn(ILBPair(_pair), amountsIn[i], ILBPair(_pair).tokenX() == _token);
             }

This calculation is implemented correctly in JoeLibrary.getAmountIn, which is used in LBQuoter. Also, Trader Joe V1 uses JoeLibrary to calculate input amounts:

• https://github.com/traderjoe-xyz/joe-core/blob/main/contracts/traderjoe/JoeRouter02.sol#L291
• https://github.com/traderjoe-xyz/joe-core/blob/main/contracts/traderjoe/JoeRouter02.sol#L319
• https://github.com/traderjoe-xyz/joe-core/blob/main/contracts/traderjoe/JoeRouter02.sol#L350

// test/audit/RouterMath1.t.sol
// SPDX-License-Identifier: UNLICENSED

pragma solidity ^0.8.7;

import "../TestHelper.sol";

import "../../src/LBRouter.sol";
import "../../src/interfaces/IJoePair.sol";

contract RouterMath1Test is TestHelper {
    IERC20 internal token;

    function setUp() public {
        token = new ERC20MockDecimals(18);
        ERC20MockDecimals(address(token)).mint(address(this), 100e18);

        router = new LBRouter(
            ILBFactory(address(0x00)),
            IJoeFactory(address(this)),
            IWAVAX(address(0x02))
        );
    }

    // Imitates V1 factory.
    function getPair(address, /*tokenX*/ address /*tokenY*/ ) public view returns (address) {
        return address(this);
    }

    // Imitates V1 pool.
    function getReserves() public pure returns (uint112, uint112, uint32) {
        return (1e18, 1e18, 0);
    }

    // Imitates V1 pool.
    function swap(uint256 amount0, uint256 amount1, address to, bytes memory data) public pure { /* NOOP */ }

    function testScenario() public {
        // Setting up a swap via one V1 pool.
        uint256[] memory steps = new uint256[](1);
        steps[0] = 0;

        IERC20[] memory path = new IERC20[](2);
        path[0] = IERC20(address(token));
        path[1] = IERC20(address(this));

        uint256 amountOut = 0.0001e18;

        token.approve(address(router), 1e18);
        uint256[] memory amountsIn =
            router.swapTokensForExactTokens(amountOut, 1e18, steps, path, address(this), block.timestamp + 1000);
        assertEq(amountsIn.length, 2);

        // This amount was calculated incorrectly.
        assertEq(amountsIn[0], 111074086415639232); // Equals to 100310933801505 when fixed.
        // This was the input amount.
        assertEq(amountsIn[1], amountOut);

        uint256 actualAmountOut = amountsIn[0];

        // Reproduce the calculations using JoeLibrary.getAmountIn. This piece:
        // https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBRouter.sol#L720-L725
        (uint256 _reserveIn, uint256 _reserveOut,) = IJoePair(address(this)).getReserves();
        if (address(token) > address(this)) {
            (_reserveIn, _reserveOut) = (_reserveOut, _reserveIn);
        }

        uint256 expectedAmountOut = JoeLibrary.getAmountIn(amountOut, _reserveIn, _reserveOut);

        // This is the correct amount.
        assertEq(expectedAmountOut, 100310933801505);

        // The wrong amount is much bigger than the expected one.
        assertEq(actualAmountOut - expectedAmountOut, 110973775481837727);

        // When output amount is too big (amountOut * 997 > _reserveOut), it reverts with an underflow.
        amountOut = 0.5 ether;
        vm.expectRevert(stdError.arithmeticError);
        router.swapTokensForExactTokens(amountOut, 1e18, steps, path, address(this), block.timestamp + 1000);
    }
}

Tools Used

Manual review.

Recommended Mitigation Steps

Consider using the JoeLibrary.getAmountIn function in the _getAmountsIn function of LBRouter when computing input amounts for V1 pools.

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/SwapHelper.sol#L59-L63 https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/SwapHelper.sol#L65-L66

Vulnerability details

Impact

Trades pay less swap fees than expected, thus liquidity providers earn less than expected.

Proof of Concept

LBPair is a liquidity pool contract that allows liquidity providers to deposit tokens and earn swap fees accrued on liquidity that's used during tokens swapping. Traders can use the liquidity provided by liquidity providers to make token swaps. Each LBPair is a pool of liquidity of two tokens, which can be swapped with each other.

Swap fees are accrued when traders swap tokens. SwapHelper.getAmounts is the function that:

1. calculates available liquidity in the current bin;
2. applies swap fees;
3. calculates the changes in input and output amounts that happen during a swap.

Swap fees are collected from the input token:

_bin.updateFees(_swapForY ? _pair.feesX : _pair.feesY, _fees, _swapForY, totalSupply(_pair.activeId));

When _swapForY is true (when selling token X to buy token Y), fees are added to _pair.feesX (which stores fees accrued in token X); when _swapForY is false, fees are added to _pair.feesY (which stores fees in token Y):

function updateFees(
    ILBPair.Bin memory bin,
    FeeHelper.FeesDistribution memory pairFees,
    FeeHelper.FeesDistribution memory fees,
    bool swapForY,
    uint256 totalSupply
) internal pure {
    pairFees.total += fees.total;
    // unsafe math is fine because total >= protocol
    unchecked {
        pairFees.protocol += fees.protocol;
    }

    if (swapForY) {
        bin.accTokenXPerShare += fees.getTokenPerShare(totalSupply);
    } else {
        bin.accTokenYPerShare += fees.getTokenPerShare(totalSupply);
    }
}

Thus, when we're selling token X and buying token Y, fees are collected from token X and added to _pair.feesX; when selling token Y and buying token X, fees are collected from token Y and added to _pair.feesY.

In Trader Joe V2, swap fees are made of two components: base fee and variable fee:

function getTotalFee(FeeParameters memory _fp) private pure returns (uint256) {
    unchecked {
        return getBaseFee(_fp) + getVariableFee(_fp);
    }
}

And both values are set as percentages with 18 decimals. Thus, to calculate fee amount we multiply input amount by a swap fee:

• https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/SwapHelper.sol#L59
• https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/SwapHelper.sol#L65

Then, we subtract the fee amount from the input amount and use the result to calculate the output amount:

• https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/SwapHelper.sol#L61-L63
• https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/SwapHelper.sol#L66-L71

However, there are two flaws in how this is implemented:

1. when _maxAmountInToBin <= amountIn, fees are collected from the reserve of the input token, not from the input amount;
2. when amountIn < _maxAmountInToBin, wrong function is used to calculate fees: getFeeAmountFrom is used instead of getFeeAmount.

In both of these situations, trader pays less fees than defined by the getTotalFee function.

// test/audit/SwapFeeUnderpaying.t.sol
// SPDX-License-Identifier: UNLICENSED

pragma solidity ^0.8.7;

import "../../src/libraries/FeeHelper.sol";

import "../TestHelper.sol";

contract SwapFeeUnderpayingTest is TestHelper {
    using FeeHelper for FeeHelper.FeeParameters;

    IERC20 internal tokenX;
    IERC20 internal tokenY;

    function setUp() public {
        tokenX = new ERC20MockDecimals(18);
        tokenY = new ERC20MockDecimals(18);

        ERC20MockDecimals(address(tokenX)).mint(address(this), 100e18);
        ERC20MockDecimals(address(tokenY)).mint(address(this), 100e18);

        factory = new LBFactory(DEV, 8e14);
        ILBPair _LBPairImplementation = new LBPair(factory);
        factory.setLBPairImplementation(address(_LBPairImplementation));
        factory.addQuoteAsset(tokenY);
        setDefaultFactoryPresets(DEFAULT_BIN_STEP);

        pair = createLBPairDefaultFees(tokenX, tokenY); // price is 1
    }

    // First scenario: buy the entire Y reserve of the bin.
    function testScenario1() public {
        // Add liquidity: 1e18 of token X and 1e18 of token Y.
        tokenX.transfer(address(pair), 1e18);
        tokenY.transfer(address(pair), 1e18);

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

        uint256[] memory distributionX = new uint256[](1);
        distributionX[0] = 1e18;

        uint256[] memory distributionY = new uint256[](1);
        distributionY[0] = 1e18;

        (,, uint256[] memory liquidityMinted) = pair.mint(ids, distributionX, distributionY, address(this));
        assertEq(liquidityMinted.length, 1);
        assertEq(liquidityMinted[0], 2e18);

        FeeHelper.FeeParameters memory fp = pair.feeParameters();
        (uint256 reserveX, uint256 reserveY,) = pair.getReservesAndId();

        // Computing an amount that will get us into this branch:
        // https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/SwapHelper.sol#L62-L63
        uint256 amountIn = reserveX + fp.getFeeAmount(reserveY / 1);

        uint256 tokenYBalanceBefore = tokenY.balanceOf(address(this));

        tokenX.transfer(address(pair), amountIn);
        pair.swap(true, address(this));

        // Bought the whole reserve Y.
        assertEq(tokenY.balanceOf(address(this)) - tokenYBalanceBefore, reserveY);

        // Expected fee: 1251562500000000. Calculated as fee percentage times input amount.
        uint256 expectedFee = ((fp.getBaseFee() + fp.getVariableFee()) * amountIn) / Constants.PRECISION;

        // Actual fee: 1250000000000000.
        (uint256 actualFee, ,,) = pair.getGlobalFees();

        // Underpaid swap fee = expected fee - actual fee.
        assertEq(expectedFee - actualFee, 1562500000000);
    }

    // Second scenario: buy a portion of the Y reserve.
    function testScenario2() public {
        // Add liquidity: 2e18 of token X and 2e18 of token Y.
        tokenX.transfer(address(pair), 2e18);
        tokenY.transfer(address(pair), 2e18);

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

        uint256[] memory distributionX = new uint256[](1);
        distributionX[0] = 1e18;

        uint256[] memory distributionY = new uint256[](1);
        distributionY[0] = 1e18;

        (,, uint256[] memory liquidityMinted) = pair.mint(ids, distributionX, distributionY, address(this));
        assertEq(liquidityMinted.length, 1);
        assertEq(liquidityMinted[0], 4e18);

        FeeHelper.FeeParameters memory fp = pair.feeParameters();

        // Buying a portion of token Y. Will end up in this branch:
        // https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/SwapHelper.sol#L65-L71
        uint256 amountIn = 1e18;

        uint256 tokenYBalanceBefore = tokenY.balanceOf(address(this));

        tokenX.transfer(address(pair), amountIn);
        pair.swap(true, address(this));

        assertEq(tokenY.balanceOf(address(this)) - tokenYBalanceBefore, 998751560549313359);

        // Expected fee: 1250000000000000.
        uint256 expectedFee = ((fp.getBaseFee() + fp.getVariableFee()) * amountIn) / Constants.PRECISION;

        // Actual fee: 1248439450686641.
        (uint256 actualFee,,,) = pair.getGlobalFees();

        // Underpaid swap fee = expected fee - actual fee.
        assertEq(expectedFee - actualFee, 1560549313359);
    }
}

Tools Used

Manual review.

Recommended Mitigation Steps

Ensure that swap fees are calculated correctly:

1. applied to the input amount;
2. fee amount is calculated using the FeeHelper.getFeeAmount function.