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

[code423n4]

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.

[GalloDaSballo]

Really high quality as well

[GalloDaSballo]

Dup of #384

[trust1995]

Not sure if this has any impact on rewards, but there is no duplicate-ISSUEID label here.
Would like to know if that is 100% fine for the reward calculation.

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[c4-judge]

GalloDaSballo marked the issue as duplicate of #384

[Simon-Busch]

Marked this issue as satisfactory as requested by @GalloDaSballo