Logical error in LBToken#_burn causes loss of access to LBTokens

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBToken.sol#L227-L247

Vulnerability details

Impact

All mints, transfers and burns of affected LBToken will revert causing user funds to be irretrievable

Proof of Concept

_beforeTokenTransfer(address(0), _account, _id, _amount);

When burning LB, _beforeTokenTransfer is called with address(0) as the from address and _account as the to address, which is reverse of what it should be. _beforeTokenTransfer is overridden in LBPair:

function _beforeTokenTransfer(
    address _from,
    address _to,
    uint256 _id,
    uint256 _amount
) internal override(LBToken) {
    unchecked {
        super._beforeTokenTransfer(_from, _to, _id, _amount);

        Bin memory _bin = _bins[_id];

        if (_from != _to) {
            if (_from != address(0) && _from != address(this)) {
                uint256 _balanceFrom = balanceOf(_from, _id);

                _cacheFees(_bin, _from, _id, _balanceFrom, _balanceFrom - _amount);
            }

            if (_to != address(0) && _to != address(this)) {
                uint256 _balanceTo = balanceOf(_to, _id);

                _cacheFees(_bin, _to, _id, _balanceTo, _balanceTo + _amount);
            }
        }
    }
}

The issue arises from the fact that _cacheFees is now called with _balanceTo + amount instead of _balanceTo - amount.

function _cacheFees(
    Bin memory _bin,
    address _user,
    uint256 _id,
    uint256 _previousBalance,
    uint256 _newBalance
) private {
    unchecked {
        bytes32 _unclaimedData = _unclaimedFees[_user];

        uint256 amountX = _unclaimedData.decode(type(uint128).max, 0);
        uint256 amountY = _unclaimedData.decode(type(uint128).max, 128);

        (uint256 _amountX, uint256 _amountY) = _getPendingFees(_bin, _user, _id, _previousBalance);

        _updateUserDebts(_bin, _user, _id, _newBalance);

        (amountX += _amountX).safe128();
        (amountY += _amountY).safe128();

        _unclaimedFees[_user] = bytes32((amountY << 128) | amountX);
    }
}

_newBalance (_balanceTo + amount) is passed into _updateUserDebts

function _updateUserDebts(
    Bin memory _bin,
    address _account,
    uint256 _id,
    uint256 _balance
) private {
    uint256 _debtX = _bin.accTokenXPerShare.mulShiftRoundDown(_balance, Constants.SCALE_OFFSET);
    uint256 _debtY = _bin.accTokenYPerShare.mulShiftRoundDown(_balance, Constants.SCALE_OFFSET);

    _accruedDebts[_account][_id].debtX = _debtX;
    _accruedDebts[_account][_id].debtY = _debtY;
}

It stores the users debt using the inflated value, which inflates the users debt value for that _id. This is not problematic for the first transaction but it breaks all user interaction with that LBToken afterwards, because the overinflated value causes _beforeTokenTransfer to revert.

function _getPendingFees(
    Bin memory _bin,
    address _account,
    uint256 _id,
    uint256 _balance
) private view returns (uint256 amountX, uint256 amountY) {
    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;
}

The culprit of the revert is LBPair#_getPendingFees. Since the debt value is overinflated the subtraction will revert due to underflow. This causes _mint/_burn/_transfer to revert due to the following chain of calls: _mint/_burn/_transfer -> _beforeTokenTransfer -> _cacheFees -> _getPendingFees. Since all those functions revert it is now basically impossible for the user to interact with any LBTokens of the affected IDs. If the users has any more LB of that ID it will be permanently stuck and unredeemable.

Example:

Assume _bin.accTokenXPerShare is 1 for ID 1. A user mints 100 LB for ID 1. Setting _debts.debtX = 100 * 1 = 100. Now the user burns 10 tokens. Because of the error, _debt.debtX = (100 + 10) * 1 = 110. Now whenever the user tries to interact with the LB (mint, burn, transfer) their value will be calculated as 90 * 1 = 90. When subtracting 90 - 110, it underflows and reverts. Unless accTokenXPerShare becomes greater than 1.22 (110 / 90) _mint/_burn/_transfer will revert.

The greater the number of swaps and the greater the burn amount, the longer this gets locked. Because the bin is only active in a specific price point, it is highly likely the funds will be locked forever.

Tools Used

Manual Review

Recommended Mitigation Steps

Reverse the order of the addresses so it is correct:

-   _beforeTokenTransfer(address(0), _account, _id, _amount);
+   _beforeTokenTransfer(_account, address(0), _id, _amount);

[trust1995]

Seems to make sense, cool find.

[GalloDaSballo]

Pretty much as good as #125

[GalloDaSballo]

Dup of #125

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[c4-judge]

GalloDaSballo marked the issue as duplicate of #108