`LBToken`s sent by a user attempting a 'manual' `mint()` can be rerouted by anyone monitoring the contract's balances or the mempool to represent their own liquidity.

[code423n4]

Lines of code

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

Vulnerability details

Description

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

To use it, a user would send ERC20 tokens to the LBPair contract, and then call LBPair.mint(). The transfer is accomplished via normal ERC20 transfer functions like safeTransfer().

However, LBPair.mint() doesn't contain functionality to perform a 'pull' from the user's accounts from a previous approval.

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

Alternatively, an attacker or bot could frontrun the call to LBPair.mint() 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 add, 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 following test, adapted from code already in the test suite (LiquidityBinPairLiquidityTest.testBurnLiquidity()), demonstrates the issue.

function testMintSteal() 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);

    // Simulates BOB acquiring tokens, or actually minting them.
    token6D.mint(BOB, amount0In);
    token18D.mint(BOB, amount1In);

    // BOB transfers the ERC20s to the Pair
    vm.startPrank(BOB);
    token6D.transfer(address(pair), amount0In);
    token18D.transfer(address(pair), amount1In);
    vm.stopPrank();

    // Having detected the transfer, ALICE submits the mint before BOB, 
    // or simply frontruns BOB's transaction
    pair.mint(_ids, _distributionX, _distributionY, ALICE);

    // Prints ALICE's free liquidity positions by bin.
    uint256[] memory amounts = new uint256[](5);
    for (uint256 i; i < 5; i++) {
        amounts[i] = pair.balanceOf(ALICE, _ids[i]);
        console.log("%s", amounts[i]);
    }
}

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]

Per the Readme The LBRouter is the main contract that user will interact with as it adds security checks. Most users shouldn't interact directly with the pair.