Trader Joe v2 contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the _transfer function of the LBToken contract, if from == to, the amount of tokens will be double counted. Consider the following scenario. User A has 100 tokens and User A transfers 100 tokens to himself. In the following calculation, _fromBalance == _toBalance == 100 _balances[_id][_from] = 100 - 100 = 0 _balances[_id][_to] = 100 + 100 = 200 At this point, user A has 200 tokens.

    function _transfer(
        address _from,
        address _to,
        uint256 _id,
        uint256 _amount
    ) internal virtual {
        uint256 _fromBalance = _balances[_id][_from];
        if (_fromBalance < _amount) revert LBToken__TransferExceedsBalance(_from, _id, _amount);

        _beforeTokenTransfer(_from, _to, _id, _amount);

        uint256 _toBalance = _balances[_id][_to];

        unchecked {
            _balances[_id][_from] = _fromBalance - _amount;
            _balances[_id][_to] = _toBalance + _amount;
        }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Change to

    function _transfer(
        address _from,
        address _to,
        uint256 _id,
        uint256 _amount
    ) internal virtual {
        uint256 _fromBalance = _balances[_id][_from];
        if (_fromBalance < _amount) revert LBToken__TransferExceedsBalance(_from, _id, _amount);

        _beforeTokenTransfer(_from, _to, _id, _amount);

        unchecked {
            _balances[_id][_from] = _fromBalance - _amount;
            _balances[_id][_to] = _balances[_id][_to]  + _amount;
        }

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L887-L896

Vulnerability details

Impact

In the _swapSupportingFeeOnTransferTokens function of the LBRouter contract, when _binStep == 0, the calculation of _amountOut is small. According to the algorithm in JoeLibrary, the denominator should be reserveIn * 1000 + amountIn * 997. This will make _amountOut smaller, thus reducing the number of tokens the user gets

                if (_binStep == 0) {
                    (uint256 _reserve0, uint256 _reserve1, ) = IJoePair(_pair).getReserves();
                    if (_token < _tokenNext) {
                        uint256 _balance = _token.balanceOf(_pair);
                        uint256 _amountOut = (_reserve1 * (_balance - _reserve0) * 997) / (_balance * 1_000);

                        IJoePair(_pair).swap(0, _amountOut, _recipient, "");
                    } else {
                        uint256 _balance = _token.balanceOf(_pair);
                        uint256 _amountOut = (_reserve0 * (_balance - _reserve1) * 997) / (_balance * 1_000);

                        IJoePair(_pair).swap(_amountOut, 0, _recipient, "");
                    }

Proof of Concept

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L887-L896

Tools Used

None

Recommended Mitigation Steps

Change to

                if (_binStep == 0) {
                    (uint256 _reserve0, uint256 _reserve1, ) = IJoePair(_pair).getReserves();
                    if (_token < _tokenNext) {
                        uint256 _balance = _token.balanceOf(_pair);
                        uint amountInWithFee = (_balance - _reserve0) * 997
                        uint256 _amountOut = (_reserve1 * amountInWithFee) / ( _reserve0 * 1000 + amountInWithFee);

                        IJoePair(_pair).swap(0, _amountOut, _recipient, "");
                    } else {
                        uint256 _balance = _token.balanceOf(_pair);
                        uint amountInWithFee = (_balance - _reserve1) * 997
                        uint256 _amountOut = (_reserve0 * amountInWithFee ) / (_reserve1 * 1_000 + amountInWithFee);

                        IJoePair(_pair).swap(_amountOut, 0, _recipient, "");
                    }

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L723-L725

Vulnerability details

Impact

In the _getAmountsIn function of the LBRouter contract, amountsIn[i - 1] is calculated incorrectly when _binStep == 0. According to the algorithm in JoeLibrary, it should be ((_reserveOut - amountOut_ )* 997), not (_reserveOut - amountOut_ * 997). This makes amountsIn very large.

            if (_binStep == 0) {
                (uint256 _reserveIn, uint256 _reserveOut, ) = IJoePair(_pair).getReserves();
                if (_token > _tokenPath[i]) {
                    (_reserveIn, _reserveOut) = (_reserveOut, _reserveIn);
                }

                uint256 amountOut_ = amountsIn[i];
                // Legacy uniswap way of rounding
                amountsIn[i - 1] = (_reserveIn * amountOut_ * 1_000) / (_reserveOut - amountOut_ * 997) + 1;

Consider that in one swap _reserveIn == 1000 , reserveOut == 1001, amountOut == amountsIn[1] == 1, and amountsIn[0] == (1000 * 1 * 1_000) / (1001 - 1 * 997) + 1 == 250001 . But actually amountsIn[0] should be (1000 * 1 * 1_000) / ((1001 - 1) * 997)) + 1 == 2. Obviously this has cost the user much more tokens. Note: In JoeV1Pair, the extra tokens are not refunded.

            if (amount0Out > 0) _safeTransfer(_token0, to, amount0Out); // optimistically transfer tokens
            if (amount1Out > 0) _safeTransfer(_token1, to, amount1Out); // optimistically transfer tokens
            if (data.length > 0) IJoeCallee(to).joeCall(msg.sender, amount0Out, amount1Out, data);
            balance0 = IERC20Joe(_token0).balanceOf(address(this));
            balance1 = IERC20Joe(_token1).balanceOf(address(this));
        }
        uint256 amount0In = balance0 > _reserve0 - amount0Out ? balance0 - (_reserve0 - amount0Out) : 0;
        uint256 amount1In = balance1 > _reserve1 - amount1Out ? balance1 - (_reserve1 - amount1Out) : 0;
        require(amount0In > 0 || amount1In > 0, "Joe: INSUFFICIENT_INPUT_AMOUNT");
        {
            // scope for reserve{0,1}Adjusted, avoids stack too deep errors
            uint256 balance0Adjusted = balance0.mul(1000).sub(amount0In.mul(3));
            uint256 balance1Adjusted = balance1.mul(1000).sub(amount1In.mul(3));
            require(balance0Adjusted.mul(balance1Adjusted) >= uint256(_reserve0).mul(_reserve1).mul(1000**2), "Joe: K");

Proof of Concept

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L723-L725 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L431-L446 https://github.com/traderjoe-xyz/joe-core/blob/main/contracts/traderjoe/JoePair.sol#L206-L219

Tools Used

None

Recommended Mitigation Steps

Change to

            if (_binStep == 0) {
                (uint256 _reserveIn, uint256 _reserveOut, ) = IJoePair(_pair).getReserves();
                if (_token > _tokenPath[i]) {
                    (_reserveIn, _reserveOut) = (_reserveOut, _reserveIn);
                }

                uint256 amountOut_ = amountsIn[i];
                // Legacy uniswap way of rounding
                amountsIn[i - 1] = (_reserveIn * amountOut_ * 1_000) / ((_reserveOut - amountOut_) * 997) + 1;

Lines of code

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

Vulnerability details

Impact

The owner of the LBFactory contract can set the flashLoanFee in setFlashLoanFee, if this transaction occurs in the same block before the flashloan, the fee paid by the flashloan users will be different. This can be used to force the flashloan users to pay higher fees.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider setting a cap on the flashloan fee, or use a flat flashloan fee

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L520-L521

Vulnerability details

Impact

In the swapAVAXForExactTokens function of the LBRouter contract, when msg.value > amountsIn[0], the parameter amountsIn[0] - msg.value of _safeTransferAVAX overflows, which causes the function to not work.

        if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value);

Proof of Concept

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L520-L521

Tools Used

None

Recommended Mitigation Steps

Change to

        if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, msg.value - amountsIn[0] );