code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBFactory.sol#L312-L328

Vulnerability details

Impact

Loss of funds and financial value of funds.

Proof of Concept

LBFactory has setLBPairIgnored() to set whether the pair is ignored or not for routing, it will make the pair unusable by the router (as per the docs/NATSPEC);

    function setLBPairIgnored(
        IERC20 _tokenX,
        IERC20 _tokenY,
        uint256 _binStep,
        bool _ignored
    ) external override onlyOwner {
        (IERC20 _tokenA, IERC20 _tokenB) = _sortTokens(_tokenX, _tokenY);

        LBPairInformation memory _LBPairInformation = _LBPairsInfo[_tokenA][_tokenB][_binStep];
        if (address(_LBPairInformation.LBPair) == address(0)) revert LBFactory__AddressZero();

        if (_LBPairInformation.ignoredForRouting == _ignored) revert LBFactory__LBPairIgnoredIsAlreadyInTheSameState();

        _LBPairsInfo[_tokenA][_tokenB][_binStep].ignoredForRouting = _ignored;

        emit LBPairIgnoredStateChanged(_LBPairInformation.LBPair, _ignored);
    }

Permalink

However, if it's called without checking the pair contract balance, it will cause the users unable to reach their assets.

Alice adds AVAX liquidity to pair XY. AVAX price is 120$
3Arrows Capital states that there is no future in AVAX, they already purchased Ethereum instead.
setLBPairIgnored() is called by the owner (for any reason)
AVAX is dumped and the price is 85$ now.
Alice tries to remove her liquidity but she can't. So she loses 35$ due to the inability to access her funds.

And if the _boolean is not set false again by the owner, Alice will not be able to see her tokens in her wallet again.

Tools Used

Manual Review

Recommended Mitigation Steps

It would be best to require to check whether the contract has a balance or not. So it can save the financial value of the funds. Else, there can be a pull implementation where the liquidity providers can claim their tokens.

Shungy commented 2 years ago

I believe this finding to be invalid.

Disabling routing does not mean making it impossible to interact with the pair. It is only used by LBQuoter, which is for reading from off-chain and determining a path. Trader Joe team is likely to still have a dedicated pair page to add/remove liquidity even if a pair is disabled from routing. And let's not forget anyone can directly interact with LBRouter or LBPair and always withdraw their liquidity.

GalloDaSballo commented 2 years ago

We have instances of the opposite finding in this contest, those are also contested but more grounded.

This one is incorrect as the check is purely "cosmetic" and no routing is blocked.

Will give it a second look but I think this one is invalid

GalloDaSballo commented 1 year ago

Closing as invalid as the list is purely informational and the finding has no basis

c4-judge commented 1 year ago

GalloDaSballo marked the issue as nullified

code-423n4 / 2022-10-traderjoe-findings