Trader Joe v2 contest - vv7's results

Lines of code

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

Vulnerability details

Impact

Owner could deny users from using flash loans.

Proof of Concept

 function setFlashLoanFee(uint256 _flashLoanFee) external override onlyOwner {
        uint256 _oldFlashLoanFee = flashLoanFee;

        if (_oldFlashLoanFee == _flashLoanFee) revert LBFactory__SameFlashLoanFee(_flashLoanFee);

        flashLoanFee = _flashLoanFee;
        emit FlashLoanFeeSet(_oldFlashLoanFee, _flashLoanFee);
    }

_flashLoanFee is not checked here, so it can be set to any value by a malicious owner.

When a user attempts to call flashLoan() from LBPair.sol the following function is called:

function _getFlashLoanFee(uint256 _amount, uint256 _fee) internal pure returns (uint256) {
        return (_amount * _fee) / Constants.PRECISION;
    }

If _fee is large, this function will revert, which will prevent a user from using flash loan functionality.

Tools Used

vim

Recommended Mitigation Steps

Add upper and lower bounds checks.

Lines of code

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

Vulnerability details

Impact

In case user sends more AVAX than necessary, this function will not work as expected and reverts due to Solidity builtin integer overflow checks.

Proof of Concept

function swapAVAXForExactTokens(
        uint256 _amountOut,
        uint256[] memory _pairBinSteps,
        IERC20[] memory _tokenPath,
        address _to,
        uint256 _deadline
    )
    external
         payable
         override
         ensure(_deadline)
         verifyInputs(_pairBinSteps, _tokenPath)
         returns (uint256[] memory amountsIn)
     {
         if (_tokenPath[0] != IERC20(wavax)) revert LBRouter__InvalidTokenPath(address(_tokenPath[0]));

         address[] memory _pairs = _getPairs(_pairBinSteps, _tokenPath);
         amountsIn = _getAmountsIn(_pairBinSteps, _pairs, _tokenPath, _amountOut);

         if (amountsIn[0] > msg.value) revert LBRouter__MaxAmountInExceeded(msg.value, amountsIn[0]);

         _wavaxDepositAndTransfer(_pairs[0], amountsIn[0]);

         uint256 _amountOutReal = _swapTokensForExactTokens(_pairs, _pairBinSteps, _tokenPath, amountsIn, _to);

         if (_amountOutReal < _amountOut) revert LBRouter__InsufficientAmountOut(_amountOut, _amountOutReal);

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

On line #520 this function will do refund, but I believe the logic is incorrect.

Tools Used

vim

Recommended Mitigation Steps

Correct call should be:

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