Connext Amarok contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L526-L616 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#L398-L428 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L345-L351

Vulnerability details

Impact

routers pay for transaction in destination domain then nomad messages come and routers get paid again. but the amount routers pay in execute() are what transaction sender signed and the amount routers receive is what nomad sends and handles in _reconcile() but this two amount can be different because of slippage and swap that happens in xcall() because the amount sent in nomad message is the result of swapToLocalAssetIfNeeded(). So it's possible for routers to lose funds if some slippage happens in that swap.

Proof of Concept

This is xcall() code:

  function xcall(XCallArgs calldata _args) external payable whenNotPaused nonReentrant returns (bytes32) {
    // Sanity checks.
    {
      // Correct origin domain.
      if (_args.params.originDomain != s.domain) {
        revert BridgeFacet__xcall_wrongDomain();
      }

      // Recipient is defined.
      if (_args.params.to == address(0)) {
        revert BridgeFacet__xcall_emptyTo();
      }

      // If callback address is not set, callback fee should be 0.
      if (_args.params.callback == address(0) && _args.params.callbackFee > 0) {
        revert BridgeFacet__xcall_nonZeroCallbackFeeForCallback();
      }

      // Callback is contract if supplied.
      if (_args.params.callback != address(0) && !Address.isContract(_args.params.callback)) {
        revert BridgeFacet__xcall_callbackNotAContract();
      }
    }

    bytes32 transferId;
    bytes memory message;
    XCalledEventArgs memory eventArgs;
    {
      // Get the remote BridgeRouter address; revert if not found.
      bytes32 remote = _mustHaveRemote(_args.params.destinationDomain);

      // Get the true transacting asset ID (using wrapper instead of native, if applicable).
      address transactingAssetId = _args.transactingAssetId == address(0)
        ? address(s.wrapper)
        : _args.transactingAssetId;

      // Check that the asset is supported -- can be either adopted or local.
      ConnextMessage.TokenId memory canonical = s.adoptedToCanonical[transactingAssetId];
      if (canonical.id == bytes32(0)) {
        // Here, the asset is *not* the adopted asset. The only other valid option
        // is for this asset to be the local asset (i.e. transferring madEth on optimism)
        // NOTE: it *cannot* be the canonical asset. the canonical asset is only used on
        // the canonical domain, where it is *also* the adopted asset.
        if (s.tokenRegistry.isLocalOrigin(transactingAssetId)) {
          // revert, using a token of local origin that is not registered as adopted
          revert BridgeFacet__xcall_notSupportedAsset();
        }

        (uint32 canonicalDomain, bytes32 canonicalId) = s.tokenRegistry.getTokenId(transactingAssetId);
        canonical = ConnextMessage.TokenId(canonicalDomain, canonicalId);
      }

      transferId = _getTransferId(_args, canonical);
      s.nonce += 1;

      // Store the relayer fee
      s.relayerFees[transferId] = _args.params.relayerFee;

      // Transfer funds of transacting asset to the contract from the user.
      // NOTE: Will wrap any native asset transferred to wrapped-native automatically.
      (, uint256 amount) = AssetLogic.handleIncomingAsset(
        _args.transactingAssetId,
        _args.amount,
        _args.params.relayerFee + _args.params.callbackFee
      );

      // Swap to the local asset from adopted if applicable.
      (uint256 bridgedAmt, address bridged) = AssetLogic.swapToLocalAssetIfNeeded(
        canonical,
        transactingAssetId,
        amount,
        _args.params.slippageTol
      );

      // Transfer callback fee to PromiseRouter if set
      if (_args.params.callbackFee != 0) {
        s.promiseRouter.initCallbackFee{value: _args.params.callbackFee}(transferId);
      }

      message = _formatMessage(_args, bridged, transferId, bridgedAmt);
      s.xAppConnectionManager.home().dispatch(_args.params.destinationDomain, remote, message);

      // Format arguments for XCalled event that will be emitted below.
      eventArgs = XCalledEventArgs({
        transactingAssetId: transactingAssetId,
        amount: amount,
        bridgedAmt: bridgedAmt,
        bridged: bridged
      });
    }

    // emit event
    emit XCalled(transferId, _args, eventArgs, s.nonce - 1, message, msg.sender);

    return transferId;
  }

As you can see it swaps what user sent to LoccalAsset which the amount is bridgedAmt and then send value of bridgedAmt to nomad bridge message = _formatMessage(_args, bridged, transferId, bridgedAmt). but the amount user signed in _args.amount is different and that what user sends to contract. the reasons that bridgedAmt could be different than _args.amount is: 1- deflationary tokens in transferring from user. 2- slippage in swap to local token. This is _reconcile() code:

  function _reconcile(uint32 _origin, bytes memory _message) internal {
    // Parse tokenId and action from the message.
    bytes29 msg_ = _message.ref(0).mustBeMessage();
    bytes29 tokenId = msg_.tokenId();
    bytes29 action = msg_.action();

    // Assert that the action is valid.
    if (!action.isTransfer()) {
      revert BridgeFacet__reconcile_invalidAction();
    }

    // Load the transferId.
    bytes32 transferId = action.transferId();

    // Ensure the transaction has not already been handled (i.e. previously reconciled).
    if (s.reconciledTransfers[transferId]) {
      revert BridgeFacet__reconcile_alreadyReconciled();
    }

    // NOTE: `tokenId` and `amount` must be in plaintext in the message so funds can *only* be minted by
    // `handle`. They are both used in the generation of the `transferId` so routers must provide them
    // correctly to be reimbursed.

    // Get the appropriate local token contract for the given tokenId on this chain.
    // NOTE: If the token is of remote origin and there is no existing representation token contract,
    // the TokenRegistry will deploy a new one.
    address token = s.tokenRegistry.ensureLocalToken(tokenId.domain(), tokenId.id());

    // Load amount once.
    uint256 amount = action.amnt();

    // Mint tokens if the asset is of remote origin (i.e. is representational).
    // NOTE: If the asset IS of local origin (meaning it's canonical), then the tokens will already be held
    // in escrow in this contract (from previous `xcall`s).
    if (!s.tokenRegistry.isLocalOrigin(token)) {
      IBridgeToken(token).mint(address(this), amount);

      // Update the recorded `detailsHash` for the token (name, symbol, decimals).
      // TODO: do we need to keep this
      bytes32 details = action.detailsHash();
      IBridgeToken(token).setDetailsHash(details);
    }

    // Mark the transfer as reconciled.
    s.reconciledTransfers[transferId] = true;

    // If the transfer was executed using fast-liquidity provided by routers, then this value would be set
    // to the participating routers.
    // NOTE: If the transfer was not executed using fast-liquidity, then the funds will be reserved for
    // execution (i.e. funds will be delivered to the transfer's recipient in a subsequent `execute` call).
    address[] memory routers = s.routedTransfers[transferId];

    // If fast transfer was made using portal liquidity, we need to repay
    // FIXME: routers can repay any-amount out-of-band using the `repayAavePortal` method
    // or by interacting with the aave contracts directly
    uint256 portalTransferAmount = s.portalDebt[transferId] + s.portalFeeDebt[transferId];

    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);
  }

As you can see it uses amount in message to calculate what router should receive. This is _handleExecuteLiquidity() code which is used in execute():

  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 it uses the amount defined in ExecuteArgs to see how much routers should pay.

because of this two issue (deflationary tokens and swap slippage) attacker could fool protocol to spend more than what he transferred to protocol. this could be seen as two bug so.

Tools Used

VIM

Recommended Mitigation Steps

update spending amount based on (deflationary tokens and swap slippage)

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/RoutersFacet.sol#L289-L325

Vulnerability details

Impact

There are some security levels for router, like setting owner and recipient and when removeRouter() is called this values set to 0x0 and router address become vulnerable. contract should transfer router balance to recipient before removing it.

Proof of Concept

This is removeRouter() code:

  /**
   * @notice Used to remove routers that can transact crosschain
   * @param router Router address to remove
   */
  function removeRouter(address router) external onlyOwner {
    // Sanity check: not empty
    if (router == address(0)) revert RoutersFacet__removeRouter_routerEmpty();

    // Sanity check: needs removal
    if (!s.routerPermissionInfo.approvedRouters[router]) revert RoutersFacet__removeRouter_notAdded();

    // Update mapping
    s.routerPermissionInfo.approvedRouters[router] = false;

    // Emit event
    emit RouterRemoved(router, msg.sender);

    // Remove router owner
    address _owner = s.routerPermissionInfo.routerOwners[router];
    if (_owner != address(0)) {
      emit RouterOwnerAccepted(router, _owner, address(0));
      // delete routerOwners[router];
      s.routerPermissionInfo.routerOwners[router] = address(0);
    }

    // Remove router recipient
    address _recipient = s.routerPermissionInfo.routerRecipients[router];
    if (_recipient != address(0)) {
      emit RouterRecipientSet(router, _recipient, address(0));
      // delete routerRecipients[router];
      s.routerPermissionInfo.routerRecipients[router] = address(0);
    }

    // Clear any proposed ownership changes
    s.routerPermissionInfo.proposedRouterOwners[router] = address(0);
    s.routerPermissionInfo.proposedRouterTimestamp[router] = 0;
  }

As you can see there is no check for router balances and owner can set all the security variables for router address to 0x0 and router balance would be in danger. There may be other functionalities that could be broke by just removing router without checking them.

Tools Used

VIM

Recommended Mitigation Steps

check some states before removing router.

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.

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/promise/PromiseRouter.sol#L226-L262

Vulnerability details

Impact

process() in PromiseRouter is used for process stored callback function and anyone calls it gets callbackFee and it calls callback() function of callbackAddress. but attacker set a callbackAddress that reverts on callback() and cause process() caller griefing. attacker can perform this buy front running or complex logic.

Proof of Concept

This is process() code:

  /**
   * @notice Process stored callback function
   * @param transferId The transferId to process
   */
  function process(bytes32 transferId, bytes calldata _message) public nonReentrant {
    // parse out the return data and callback address from message
    bytes32 messageHash = messageHashes[transferId];
    if (messageHash == bytes32(0)) revert PromiseRouter__process_invalidTransferId();

    bytes29 _msg = _message.ref(0).mustBePromiseCallback();
    if (messageHash != _msg.keccak()) revert PromiseRouter__process_invalidMessage();

    // enforce relayer is whitelisted by calling local connext contract
    if (!connext.approvedRelayers(msg.sender)) revert PromiseRouter__process_notApprovedRelayer();

    address callbackAddress = _msg.callbackAddress();

    if (!AddressUpgradeable.isContract(callbackAddress)) revert PromiseRouter__process_notContractCallback();

    uint256 callbackFee = callbackFees[transferId];

    // remove message
    delete messageHashes[transferId];

    // remove callback fees
    callbackFees[transferId] = 0;

    // execute callback
    ICallback(callbackAddress).callback(transferId, _msg.returnSuccess(), _msg.returnData());

    emit CallbackExecuted(transferId, msg.sender);

    // Should transfer the stored relayer fee to the msg.sender
    if (callbackFee > 0) {
      AddressUpgradeable.sendValue(payable(msg.sender), callbackFee);
    }
  }

As you can see it calls ICallback(callbackAddress).callback(transferId, _msg.returnSuccess(), _msg.returnData()); and if that call reverts then whole transaction would revert. so attacker can setup callbackAddress that revert and the caller of process() wouldn't get any fee and lose gas.

Tools Used

VIM

Recommended Mitigation Steps

change the code so it won't revert if call to callbackAddress reverts.

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/SponsorVault.sol#L187-L220

Vulnerability details

Impact

when code swaps tokens it should specify slippage but in reimburseLiquidityFees() code contract calls tokenExchange.swapExactIn() without slippage and it's possible to perform sandwich attack and make contract to swap on bad exchange rates and there is MEV.

Proof of Concept

This is reimburseLiquidityFees() code in SponserVault :

  /**
   * @notice Performs liquidity fee reimbursement.
   * @dev Uses the token exchange or liquidity deposited in this contract.
   *      The `_receiver` address is only used for emitting in the event.
   * @param _token The address of the token
   * @param _liquidityFee The liquidity fee amount
   * @param _receiver The address of the receiver
   * @return Sponsored liquidity fee amount
   */
  function reimburseLiquidityFees(
    address _token,
    uint256 _liquidityFee,
    address _receiver
  ) external override onlyConnext returns (uint256) {
    uint256 sponsoredFee;

    if (address(tokenExchanges[_token]) != address(0)) {
      uint256 currentBalance = address(this).balance;
      ITokenExchange tokenExchange = tokenExchanges[_token];

      uint256 amountIn = tokenExchange.getInGivenExpectedOut(_token, _liquidityFee);
      amountIn = currentBalance >= amountIn ? amountIn : currentBalance;

      // sponsored fee may end being less than _liquidityFee due to slippage
      sponsoredFee = tokenExchange.swapExactIn{value: amountIn}(_token, msg.sender);
    } else {
      uint256 balance = IERC20(_token).balanceOf(address(this));
      sponsoredFee = balance < _liquidityFee ? balance : _liquidityFee;

      // some ERC20 do not allow to transfer 0 amount
      if (sponsoredFee > 0) {
        IERC20(_token).safeTransfer(msg.sender, sponsoredFee);
      }
    }

    emit ReimburseLiquidityFees(_token, sponsoredFee, _receiver);

    return sponsoredFee;
  }

As you can see there is no slippage defined when calling swapExactIn() can that swap could happen in any exchange rate. it's possible to perform sandwich attack and do large swap before and after the transaction and make users lose funds. and it's also MEV opportunity.

Tools Used

VIM

Recommended Mitigation Steps

specify slippage when calling swap tokens.

In all of proxy pattern contracts (BridgeToken, LPToken, StableSwap, TokenRegistry, PromiseRouter,...) the implementation contract don't initialized

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/LPToken.sol#L13-L27 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/StableSwap.sol#L31-L116

Vulnerability details

Impact

In proxy pattern if the initialization contract don't get initialized then attacker can initialize it and get access to it's admin level functionalities and with logics like self-destruct it can harm the protocol. it's safer to initialize all the implementation contracts too.

Proof of Concept

There is no constructor in contracts to set the state of implementation contract to initialized in BridgeToken, LPToken, StableSwap, TokenRegistry, PromiseRouter and others.. attacker can call initialize() method for implementation contract and take control of it and if there were some logics like self-destruct or ... attacker can use it harm protocol.

Tools Used

VIM

Recommended Mitigation Steps

add constructor and set implementation contract state to initialized