0x73696d616f - BancorExchangeProvider::executeSwap() may set a supply of 0 which will brick the contract

[sherlock-admin4]

0x73696d616f

Medium

BancorExchangeProvider::executeSwap() may set a supply of 0 which will brick the contract

Summary

BancorExchangeProvider::executeSwap() allows setting a supply or reserve of 0 which will brick the contract as all formulas in BancorFormula for swapping tokens revert if the supply or reserve is 0, which will lead to redeployment.

Root Cause

In BancorExchangeProvider:286, the supply or reserve may be set to 0.
In BancorFormula:199,200,246,257,290,291,333,334 it reverts if supply or reserve are 0.
The contracts following the Broker::swapIn() or Broker::swapOut() calls flow allow swapping and making the supply or reserve 0.

Internal pre-conditions

None.

External pre-conditions

None.

Attack Path

1. Users swap all reserve or supply of the exchange.

Impact

The protocol is bricked and has to be redeployed.

PoC

Explaining the flow for swapping in only, as swapping out is similar.
Broker::swapIn() does not deal with a swap resulting in 0 supply or reserve.
Broker::swapIn():

function swapIn(
  address exchangeProvider,
  bytes32 exchangeId,
  address tokenIn,
  address tokenOut,
  uint256 amountIn,
  uint256 amountOutMin
) external nonReentrant returns (uint256 amountOut) {
  require(isExchangeProvider[exchangeProvider], "ExchangeProvider does not exist");
  // slither-disable-next-line reentrancy-benign
  amountOut = IExchangeProvider(exchangeProvider).swapIn(exchangeId, tokenIn, tokenOut, amountIn);
  require(amountOut >= amountOutMin, "amountOutMin not met");
  guardTradingLimits(exchangeId, tokenIn, amountIn, tokenOut, amountOut);

  address reserve = exchangeReserve[exchangeProvider];
  transferIn(payable(msg.sender), tokenIn, amountIn, reserve);
  transferOut(payable(msg.sender), tokenOut, amountOut, reserve);
  emit Swap(exchangeProvider, exchangeId, msg.sender, tokenIn, tokenOut, amountIn, amountOut);
}

It calls GoodDollarExchangeProvider::swapIn(), which also does not prevent this scenario:

function swapIn(
  bytes32 exchangeId,
  address tokenIn,
  address tokenOut,
  uint256 amountIn
) public override onlyBroker whenNotPaused returns (uint256 amountOut) {
  amountOut = BancorExchangeProvider.swapIn(exchangeId, tokenIn, tokenOut, amountIn);
}

It then calls BancorExchangeProvider.swapIn(), which again allows swapping all supply or reserve:

function swapIn(
  bytes32 exchangeId,
  address tokenIn,
  address tokenOut,
  uint256 amountIn
) public virtual onlyBroker returns (uint256 amountOut) {
  PoolExchange memory exchange = getPoolExchange(exchangeId);
  uint256 scaledAmountIn = amountIn * tokenPrecisionMultipliers[tokenIn];
  uint256 scaledAmountOut = _getScaledAmountOut(exchange, tokenIn, tokenOut, scaledAmountIn);
  executeSwap(exchangeId, tokenIn, scaledAmountIn, scaledAmountOut);

  amountOut = scaledAmountOut / tokenPrecisionMultipliers[tokenOut];
  return amountOut;
}

BancorExchangeProvider::_getScaledAmountOut() does not deal with this and lastly BancorFormula::purchaseTargetAmount() and BancorFormula::saleTargetAmount() allow swapping all supply or reserve.

function purchaseTargetAmount(
  uint256 _supply,
  uint256 _reserveBalance,
  uint32 _reserveWeight,
  uint256 _amount
) internal view returns (uint256) {
  // validate input
  require(_supply > 0, "ERR_INVALID_SUPPLY");
  require(_reserveBalance > 0, "ERR_INVALID_RESERVE_BALANCE");
  require(_reserveWeight > 0 && _reserveWeight <= MAX_WEIGHT, "ERR_INVALID_RESERVE_WEIGHT");

  // special case for 0 deposit amount
  if (_amount == 0) return 0;

  // special case if the weight = 100%
  if (_reserveWeight == MAX_WEIGHT) return (_supply * _amount) / _reserveBalance;

  uint256 result;
  uint8 precision;
  uint256 baseN = _amount + _reserveBalance;
  (result, precision) = power(baseN, _reserveBalance, _reserveWeight, MAX_WEIGHT);
  uint256 temp = (_supply * result) >> precision;
  return temp - _supply;
}

...

  function saleCost(
    uint256 _supply,
    uint256 _reserveBalance,
    uint32 _reserveWeight,
    uint256 _amount
  ) internal view returns (uint256) {
    // validate input
    require(_supply > 0, "ERR_INVALID_SUPPLY");
    require(_reserveBalance > 0, "ERR_INVALID_RESERVE_BALANCE");
    require(_reserveWeight > 0 && _reserveWeight <= MAX_WEIGHT, "ERR_INVALID_RESERVE_WEIGHT");

    require(_amount <= _reserveBalance, "ERR_INVALID_AMOUNT");

    // special case for 0 sell amount
    if (_amount == 0) return 0;

    // special case for selling the entire supply
    if (_amount == _reserveBalance) return _supply;

    // special case if the weight = 100%
    // base formula can be simplified to:
    // Formula: amountIn = amountOut * supply / reserveBalance
    // the +1 and -1 are to ensure that this function rounds up which is required to prevent protocol loss.
    if (_reserveWeight == MAX_WEIGHT) return (_supply * _amount - 1) / _reserveBalance + 1;

    uint256 result;
    uint8 precision;
    uint256 baseD = _reserveBalance - _amount;
    (result, precision) = power(_reserveBalance, baseD, _reserveWeight, MAX_WEIGHT);
    uint256 temp1 = _supply * result;
    uint256 temp2 = _supply << precision;
    return (temp1 - temp2 - 1) / result + 1;
  }

Mitigation

Deal with users swapping the whole supply or reserve, for example by always keeping a minimum amount.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
mento-protocol/mento-core#557