division rounding error in _handleExecuteLiquidity() and _reconcile() make routerBalances and contract fund balance to get out of sync and cause fund lose

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L753-L803
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L526-L616

Vulnerability details

Impact

variable routerBalances suppose to keep track of routers balance in contract and routers can withdraw their balance from contract. but because of division rounding error in _handleExecuteLiquidity() and _reconcile() contract uses more of its tokens than it subtract from router's balance. this can lead to fund lose.

Proof of Concept

This is _handleExecuteLiquidity() code:

  /**
   * @notice Execute liquidity process used when calling `execute`
   * @dev Need this to prevent stack too deep
   */
  function _handleExecuteLiquidity(
    bytes32 _transferId,
    bool _isFast,
    ExecuteArgs calldata _args
  ) private returns (uint256, address) {
    uint256 toSwap = _args.amount;

    // If this is a fast liquidity path, we should handle deducting from applicable routers' liquidity.
    // If this is a slow liquidity path, the transfer must have been reconciled (if we've reached this point),
    // and the funds would have been custodied in this contract. The exact custodied amount is untracked in state
    // (since the amount is hashed in the transfer ID itself) - thus, no updates are required.
    if (_isFast) {
      uint256 pathLen = _args.routers.length;

      // Calculate amount that routers will provide with the fast-liquidity fee deducted.
      toSwap = _getFastTransferAmount(_args.amount, s.LIQUIDITY_FEE_NUMERATOR, s.LIQUIDITY_FEE_DENOMINATOR);

      // Save the addressess of all routers providing liquidity for this transfer.
      s.routedTransfers[_transferId] = _args.routers;

      // If router does not have enough liquidity, try to use Aave Portals.
      // only one router should be responsible for taking on this credit risk, and it should only
      // deal with transfers expecting adopted assets (to avoid introducing runtime slippage)
      if (
        !_args.params.receiveLocal &&
        pathLen == 1 &&
        s.routerBalances[_args.routers[0]][_args.local] < toSwap &&
        s.aavePool != address(0)
      ) {
        if (!s.routerPermissionInfo.approvedForPortalRouters[_args.routers[0]])
          revert BridgeFacet__execute_notApprovedForPortals();

        // Portal provides the adopted asset so we early return here
        return _executePortalTransfer(_transferId, toSwap, _args.local, _args.routers[0]);
      } else {
        // for each router, assert they are approved, and deduct liquidity
        uint256 routerAmount = toSwap / pathLen;
        for (uint256 i; i < pathLen; ) {
          // decrement routers liquidity
          s.routerBalances[_args.routers[i]][_args.local] -= routerAmount;

          unchecked {
            i++;
          }
        }
      }
    }

As you can see in last part it uses uint256 routerAmount = toSwap / pathLen to calculate amount to decrease from router's balance but because of division rounding error contract using toSwap amount of token buy total value it decrease from router's balances are lower than toSwap.

Of course in _reconcile() contract add some amount to router's balances again with division rounding error, but because the amounts are different in this two functions (in _handleExecuteLiquidity() we subtract fee and in _reconcile() we don't subtract fee) so the division rounding error would be different for them and in the end sum of total balances of routers would not be equal to contract balance. this bug could be more serious for tokens with low precision and higher price.

Tools Used

VIM

Recommended Mitigation Steps

perform better calculations.

[LayneHaber]

The maximum fund loss in this case is capped by the maximum number of routers allowed in a transfer (this will generally be below 10, meaning maximum loss from one of these errors is 18 wei units of the token -- assuming it hit max on both execute and reconcile).

I understand the rounding error exists, but even with tokens at a precision of 6 with a high price the maximum rounding error is small (i.e. 100K coin, 6 decimals, this amounts to $1.8). At this level of impact, this should amount to a value leak.

[0xleastwood]

I'm gonna downgrade this to QA because the value leak is mostly negligible and extremely hard to avoid. You could sweep the remaining balance and delegate that to the last router, but this is unfair to other routers.

[0xleastwood]

I think a good solution would be to make the last router pay the swept balance and then be reconciled this amount once the bridge transfer is complete.

[0xleastwood]

Actually, upon thinking about this more, I think there is potential for the system to not work as intended under certain parts of the transfer flow.

If the bridge transfer amount is 10 DAI and the liquidity must be provided three routers, each router provides 3 DAI respectively. If the user opted to receive the local asset than 10 DAI will be directly sent out to them. Therefore, until the transfer has been reconciled, the protocol will essentially take on temporary bad debt which won't be resolved until the bridge transfer has been reconciled. This may limit withdrawals for other routers during this period. For these reasons, I think medium severity makes more sense:)