Precision Loss During Division

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L757
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L541

Vulnerability details

Proof-of-Concept

Assume that toSwap = 10 DAI and pathLen = 3 in this example.

Therefore, the bridge will pull 10 DAI from the RouterFacet contract. However, within the BridgeFacet._handleExecuteLiquidity function, due to precision loss when solidity handles division of integer, the following code will result in routerAmount to be evaluted to 3:

// @audit-issue (10 / 3 = 3)
uint256 routerAmount = toSwap / pathLen;

The code will then proceed to deduct the liquidity from all the 3 routers equally:

• Router A = -3 DAI

• Router B = -3 DAI

• Router C = -3 DAI

A total of -9 DAI is deducted from the 3 routers. However, this value does not match with the amount of DAI that was being pulled from the RouterFacet contract. 10 DAI was pulled from RouterFacet contract, but 9 DAI was deducted from the 3 routers. Thus, there is some discrepancy over here.

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L757

  function _handleExecuteLiquidity(
    bytes32 _transferId,
    bool _isFast,
    ExecuteArgs calldata _args
  ) private returns (uint256, address) {
    uint256 toSwap = _args.amount;
		..SNIP..
        // 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++;
          }
        }
	..SNIP..

The following code within BridgeFacet._reconcile function exhibits the same problem and also demostrate that only 9 DAI will be reimbuse back to the 3 routers.

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L541

  function _reconcile(uint32 _origin, bytes memory _message) internal {
	..SNIP..
    uint256 toDistribute = amount;
    uint256 pathLen = routers.length;
    if (portalTransferAmount != 0) {
      // ensure a router took on credit risk
      if (pathLen != 1) revert BridgeFacet__reconcile_noPortalRouter();
      toDistribute = _reconcileProcessPortal(amount, token, routers[0], transferId);
    }

    if (pathLen != 0) {
      // fast liquidity path
      // Credit each router that provided liquidity their due 'share' of the asset.
      uint256 routerAmt = toDistribute / pathLen;
      for (uint256 i; i < pathLen; ) {
        s.routerBalances[routers[i]][token] += routerAmt;
        unchecked {
          i++;
        }
      }
    }

    emit Reconciled(transferId, _origin, routers, token, amount, msg.sender);
  }

Impact

It might cause some internal accounting issue within the protocol due to the discrepancy.

Recommended Mitigation Steps

It is recommended to update the code so that the amount pulled from RouterFacet contract is equal to the amount deducted from the routers. Considering having the last router "sweep" all the remaining balance as shown below:

  function _handleExecuteLiquidity(
    bytes32 _transferId,
    bool _isFast,
    ExecuteArgs calldata _args
  ) private returns (uint256, address) {
		..SNIP..
        uint256 routerAmount = toSwap / pathLen;
        uint256 remainingBalance = toSwap;
        for (uint256 i; i < pathLen - 1; ) {
          s.routerBalances[_args.routers[i]][_args.local] -= routerAmount;
          remainingBalance -= routerAmount
          unchecked { i++; }
        }
        s.routerBalances[_args.routers[pathLen-1]][_args.local] = remainingBalance;
	..SNIP..

The above code will deduct the liquidity from the routers as follows, which will add up to the 10 DAI.

• Router A = -3 DAI

• Router B = -3 DAI

• Router C = -4 DAI

[jakekidd]

Why this is a valid issue and not just a "dusting problem": In the example, the contract would be sending 10 DAI, debiting 3 DAI from each router... essentially, this extra 1 DAI (the "dust") would create an imbalance/deficiency between the liquidity tracked for all routers and the actual liquidity that the contract has.

This deficiency would be resolved when the reimbursement happens, of course. However, the more concerning problem here: if the extra 1 DAI isn't available (as in, there is literally exactly 9 DAI in the contract), this call will revert. Obviously it wouldn't be sent to chain in that event, but still unideal.

[jakekidd]

Fixed by connext/monorepo@704f87f

[0xleastwood]

Duplicate of #213.