Trader Joe v2 contest - KingNFT's results

Lines of code

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

Vulnerability details

Impact

The '_setFeesParameters' function of LBPair.sol is not properly implemented, the static and dynamic fee parameters are overlapped due to missing of 114 bits shift for dynamic parameters. It will messes up the parameters and make the LBPair contract unavailable。

Proof of Concept

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

Tools Used

VS Code

Recommended Mitigation Steps

    function _setFeesParameters(bytes32 _packedFeeParameters) internal {
        // ...
        assembly {
            sstore(_feeParameters.slot, or(_newFeeParameters, shl(_OFFSET_VARIABLE_FEE_PARAMETERS, _varParameters)))
        }
    }

Lines of code

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

Vulnerability details

Impact

The '_burn' function of LBToken.sol call internal virtual function '_beforeTokenTransfer' with wrong order for parameter 'from' and 'to'. It causes the implementation of '_beforeTokenTransfer' in LBPaire.sol to update user's debt incorrectly, cause the system corrupted.

Proof of Concept

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

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

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

Tools Used

Recommended Mitigation Steps

function _burn(
    address _account,
    uint256 _id,
    uint256 _amount
) internal virtual {
   // ...
    _beforeTokenTransfer(_account, address(0), _id, _amount);
   // ...
}

Lines of code

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

Vulnerability details

Impact

The implementation of 'flashLoan' in LBPair.sol allow users to loan as much as up to all funds in pair, but loan fees are credited to the active bin only, there is a chance a little reserve in the active bin getting unreasonably high ROI. It's recommended to distribute fees to both active bin and these adjacent ones based on their reserves. This way we incentivize liquidity providers to actively manage liquidity while ensuring fairness.

Proof of Concept

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

Tools Used

Manually review - VS code

Recommended Mitigation Steps

Adding new internal function '_distributeLoanFee' and fix 'flashLoan' as follow:

    function _distributeLoanFee(
        uint24 _id,
        FeeHelper.FeeParameters memory _fp,
        FeeHelper.FeesDistribution memory _fees,
        uint256 _amountOut,
        bool _loanForY
    ) internal {
        uint256 _left = _amountOut;
        while (_left != 0) {
            uint256 _reserve = _loanForY ? _bins[_id].reserveY : _bins[_id].reserveX;
            if (_reserve == 0) {
                _id = _tree.findFirstBin(_id, _loanForY);
                continue;
            }

            uint256 _amount;
            if (_left <= _reserve) {
                _amount = _left;
                _left = 0;
            } else {
                _amount = _reserve;
                unchecked {
                    _left -= _reserve;
                }
            }

            FeeHelper.FeesDistribution memory _feesBin = _fp.getFeeAmountDistribution(_amount * _fees.total / _amountOut);
            if (_loanForY) {
                _bins[_id].accTokenYPerShare += _feesBin.getTokenPerShare(totalSupply(_id));
            } else {
                _bins[_id].accTokenXPerShare += _feesBin.getTokenPerShare(totalSupply(_id));
            }
            if (_left != 0) {
                _id = _tree.findFirstBin(_id, _loanForY);
            }
        }
    }


    function flashLoan(
        address _to,
        uint256 _amountXOut,
        uint256 _amountYOut,
        bytes calldata _data
    ) external override nonReentrant {
        (uint256 _reserveX, uint256 _reserveY, uint256 _id) = _getReservesAndId();
        if (_amountXOut > _reserveX) revert LBPair__InsufficientReserveX();
        if (_amountYOut > _reserveY) revert LBPair__InsufficientReserveY();

        uint256 _fee = factory.flashLoanFee();
        FeeHelper.FeeParameters memory _fp = _feeParameters;
        FeeHelper.FeesDistribution memory _feesX;
        FeeHelper.FeesDistribution memory _feesY;
        if (_amountXOut != 0) { // gas saving, since it's common to loan only one of the pair
            _feesX = _fp.getFeeAmountDistribution(_getFlashLoanFee(_amountXOut, _fee));
            tokenX.safeTransfer(_to, _amountXOut);
        }
        if (_amountYOut != 0) {
            _feesY = _fp.getFeeAmountDistribution(_getFlashLoanFee(_amountYOut, _fee));
            tokenY.safeTransfer(_to, _amountYOut);
        }

        ILBFlashLoanCallback(_to).LBFlashLoanCallback(
            msg.sender,
            _amountXOut,
            _amountYOut,
            _feesX.total,
            _feesY.total,
            _data
        );

        if (_amountXOut != 0) {
            _feesX.flashLoanHelper(_pairInformation.feesX, tokenX, _reserveX);
            _distributeLoanFee(_id, _fp, _feesX, _amountXOut, false);
        }
        if (_amountYOut != 0) {
            _feesY.flashLoanHelper(_pairInformation.feesY, tokenY, _reserveY);
            _distributeLoanFee(_id, _fp, _feesY, _amountYOut, true);
        }

        emit FlashLoan(msg.sender, _to, _amountXOut, _amountYOut, _feesX.total, _feesY.total);
    }

1. Missing check if the last bin in the bin tree level 0 The function 'findFirstBin' in TreeMath.sol checks if the last bin for level 2 and level 1, but missing check for level 0. Functionally there will be no problem，since coincidentally in this case an arithmetic overflow/underflow error will occur while calling function 'closestBit'. But it will revert with an unexpected reason rather than 'TreeMath__ErrorDepthSearch'.

   function findFirstBin(
        mapping(uint256 => uint256)[3] storage _tree,
        uint24 _binId,
        bool _rightSide
    ) internal view returns (uint24) {
        unchecked {
            uint256 current;
            uint256 bit;

            (_binId, bit) = _getIdsFromAbove(_binId);

            // Search in depth 2
            if ((_rightSide && bit != 0) || (!_rightSide && bit != 255)) {
                current = _tree[2][_binId];
                bit = current.closestBit(uint8(bit), _rightSide);

                if (bit != type(uint256).max) {
                    return _getBottomId(_binId, uint24(bit));
                }
            }

            (_binId, bit) = _getIdsFromAbove(_binId);

            // Search in depth 1
            if ((_rightSide && bit != 0) || (!_rightSide && bit != 255)) {
                current = _tree[1][_binId];
                bit = current.closestBit(uint8(bit), _rightSide);

                if (bit != type(uint256).max) {
                    _binId = _getBottomId(_binId, uint24(bit));
                    current = _tree[2][_binId];

                    return _getBottomId(_binId, current.significantBit(_rightSide));
                }
            }

            // @audit missing check here
            // Search in depth 0
            current = _tree[0][0];
            bit = current.closestBit(uint8(_binId), _rightSide);
            if (bit == type(uint256).max) revert TreeMath__ErrorDepthSearch();

            current = _tree[1][bit];
            _binId = _getBottomId(uint24(bit), current.significantBit(_rightSide));

            current = _tree[2][_binId];
            return _getBottomId(_binId, current.significantBit(_rightSide));
        }
    }

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/libraries/TreeMath.sol#L56