ElasticSwap contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

Conditions should be inclusive >= or <= :

  require(
      baseTokenQty > _baseTokenQtyMin,
      "MathLib: INSUFFICIENT_BASE_TOKEN_QTY"
  );
  require(
      quoteTokenQty > _quoteTokenQtyMin,
      "MathLib: INSUFFICIENT_QUOTE_TOKEN_QTY"
  );
  require(
      _baseTokenQtyMin < maxBaseTokenQty,
      "MathLib: INSUFFICIENT_DECAY"
  );
  require(
      _quoteTokenQtyMin < maxQuoteTokenQty,
      "MathLib: INSUFFICIENT_DECAY"
  );

Otherwise, these functions will fail when e.g. baseTokenQty = _baseTokenQtyMin when the end-user expects it to pass through.

Handle

pauliax

Vulnerability details

Impact

You say: FEE_ON_TRANSFER_NOT_SUPPORTED However, this check is only triggered when the balance of baseToken is 0 (isExchangeEmpty is true). If someone directly sends some dust tokens to the contract, this check will not be triggered.

  bool isExchangeEmpty =
      IERC20(baseToken).balanceOf(address(this)) == 0;

  // transfer base tokens to Exchange
  IERC20(baseToken).safeTransferFrom(
      msg.sender,
      address(this),
      tokenQtys.baseTokenQty
  );

  if (isExchangeEmpty) {
      require(
          IERC20(baseToken).balanceOf(address(this)) ==
              tokenQtys.baseTokenQty,
          "Exchange: FEE_ON_TRANSFER_NOT_SUPPORTED"
      );
  }

Also, quoteToken is not checked for the same condition:

  // transfer quote tokens to Exchange
  IERC20(quoteToken).safeTransferFrom(
      msg.sender,
      address(this),
      tokenQtys.quoteTokenQty
  );

In my opinion, it would be nice to ensure that both tokens were transferred without losses. You can even introduce a helper function that performs these transfers and checks.

Recommended Mitigation Steps

I suggest checking balances before/after:

  uint256 balanceBefore = IERC20(baseToken).balanceOf(address(this));

  // transfer base tokens to Exchange
  IERC20(baseToken).safeTransferFrom(
      msg.sender,
      address(this),
      tokenQtys.baseTokenQty
  );

  uint256 balanceAfter = IERC20(baseToken).balanceOf(address(this));
  require(balanceAfter == balanceBefore + tokenQtys.baseTokenQty, "Exchange: FEE_ON_TRANSFER_NOT_SUPPORTED"); // could be >=

Handle

pauliax

Vulnerability details

Impact

Using the unchecked keyword to avoid redundant arithmetic checks and save gas when an underflow/overflow cannot happen, e.g.:

    if (rootK > rootKLast) {
        uint256 numerator =
            _totalSupplyOfLiquidityTokens * (rootK - rootKLast);

rootK - rootKLast will never underflow here.

Handle

pauliax

Vulnerability details

Impact

Result of this.totalSupply() could be cached to avoid duplicate calls:

  require(this.totalSupply() > 0, "Exchange: INSUFFICIENT_LIQUIDITY");
  ...
  uint256 totalSupplyOfLiquidityTokens = this.totalSupply();

Handle

pauliax

Vulnerability details

Impact

Would be cheapier to have >= here when quoteTokenQtyToReturn = internalBalances.quoteTokenReserveQty to skip math operation:

    // We should ensure no possible overflow here.
    if (quoteTokenQtyToReturn > internalBalances.quoteTokenReserveQty) {
        internalBalances.quoteTokenReserveQty = 0;
    } else {
        internalBalances.quoteTokenReserveQty -= quoteTokenQtyToReturn;
    }