Swap fees are undercollected

[code423n4]

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.

[0x0Louis]

This is a duplicate of #470

[c4-judge]

GalloDaSballo marked the issue as duplicate of #470

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[Simon-Busch]

Marked this issue as satisfactory as requested by @GalloDaSballo