h2134 - User cannot swap out all of the reserve tokens in Reserve or complete a desired swap in a single transaction by calling swapOut()

[sherlock-admin2]

h2134

Medium

User cannot swap out all of the reserve tokens in Reserve or complete a desired swap in a single transaction by calling swapOut()

Summary

When swapOut() is called by Broker, protocol will execute a token swap with fixed amountOut.

BancorExchangeProvider.sol#L208-L221:

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

    amountIn = scaledAmountIn / tokenPrecisionMultipliers[tokenIn];
    return amountIn;
  }

_getScaledAmountIn() is called to compute the required input token amount. If the swapped out token is the reserve token, then exit contribution is applied and the amountOut is updated.

BancorExchangeProvider.sol#L301-L314:

  function _getScaledAmountIn(
    PoolExchange memory exchange,
    address tokenIn,
    address tokenOut,
    uint256 scaledAmountOut
  ) internal view verifyExchangeTokens(tokenIn, tokenOut, exchange) returns (uint256 scaledAmountIn) {
    if (tokenIn == exchange.reserveAsset) {
      scaledAmountIn = fundCost(exchange.tokenSupply, exchange.reserveBalance, exchange.reserveRatio, scaledAmountOut);
    } else {
@>   // apply exit contribution
@>    scaledAmountOut = (scaledAmountOut * MAX_WEIGHT) / (MAX_WEIGHT - exchange.exitContribution);
@>    scaledAmountIn = saleCost(exchange.tokenSupply, exchange.reserveBalance, exchange.reserveRatio, scaledAmountOut);
    }
  }

Then saleCost() is called to calculate the amountIn of input token for the given amountOut.

BancorFormula.sol#L326-L358:

  function saleCost(
    uint256 _supply,
    uint256 _reserveBalance,
    uint32 _reserveWeight,
    uint256 _amount
  ) internal view returns (uint256) {
    // validate input
    ...

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

    ...
  }

As can be seen from above, there is validation to ensure the updated amountOut (_amount) is no larger than reserve balance, or the transaction would revert.

The fact is though, the updated amountOut is not the actual output token amount to be swapped out, and this validation is problematic because $1)$ it prevents user swapped out all of the reserve tokens and $2)$ user may not be able to swap out of a portion of reserve token balance in a single transaction (see details in Impact section).

Root Cause

Improper validation in saleCost().

BancorFormula.sol#L337:

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

Internal pre-conditions

Broker trading net flow limit is larger than the swap amount or no limit at all.

External pre-conditions

No response

Attack Path

No response

Impact

According to README, the exit contribution fee cannot be set to 0.

exchange.exitContribution = Between 1 and 1e8

1. User can not swap out all of the reserve tokens in Reserve.
   When saleCost() is called, _amount is always larger than the amountOut specified by a user because of exit contribution fee, when the user tries to swap out all of the reserve tokens in Reserve, they need to specify amountOut to be _reserveBalance, then _amount received by saleCost() is larger than _reserveBalance, hence the transaction will revert.

2. User can not swap out a large amount of reserve token in a single transaction.
   Suppose the reserve balance is $1000$ exit contribution fee rate is $10%$, when user tries to swap out 91% of the total reserve tokens in Reserve, amountOut is $910$ and is updated to $1011$ (scaledAmountOut = (scaledAmountOut * MAX_WEIGHT) / (MAX_WEIGHT - exchange.exitContribution)) when saleCost() is called due to exit contribution fee, and the transaction is reverted. As a result, user has to swap multiple times to complete the whole swap and waste gas fees.

PoC

Please run the PoC in GoodDollarExchangeProvider.t.sol:

  function testAudit_swapOutRevert() public {
    vm.prank(exchangeProvider.AVATAR());
    exchangeProvider.setExitContribution(exchangeId, 1e7);

    vm.startPrank(brokerAddress);
    IBancorExchangeProvider.PoolExchange memory exchange = exchangeProvider.getPoolExchange(exchangeId);

    // User cannot swap out all of the reserve tokens
    vm.expectRevert("ERR_INVALID_AMOUNT");
    exchangeProvider.swapOut(exchangeId, address(token), address(reserveToken), exchange.reserveBalance);

    // User cannot swap out a portion of the reserve token balance
    vm.expectRevert("ERR_INVALID_AMOUNT");
    exchangeProvider.swapOut(exchangeId, address(token), address(reserveToken), exchange.reserveBalance * 91 / 100);

    vm.stopPrank();
  }

Mitigation

It is recommended to add user specified amountOut as a parameter to saleCost() function, and use this value to validate against _reserveBalance:

  function saleCost(
    uint256 _supply,
    uint256 _reserveBalance,
    uint32 _reserveWeight,
    uint256 _amount,
+   uint256 _amountOut,  
  ) internal view returns (uint256) {
    ...

-   require(_amount <= _reserveBalance, "ERR_INVALID_AMOUNT");
+   require(_amountOut <= _reserveBalance, "ERR_INVALID_AMOUNT");

    ...
  }