Connext Amarok contest - xiaoming90's results

Lines of code

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

Vulnerability details

Proof-of-Concept

Assume the following:

• For simplity sake, only two (2) routers exist within Connext. Gas, relayer, callback fees and slippage are ignored.
• An attacker owns Router A. Router A has 1,000,000 oUSDC on Optimism Domain/Chain
• Router B has only 100 oUSDC on Optimism Domain/Chain
• The liquidity fee is 5% for fast transfer service
• SponserVault will reimbursed 50% of the liquidity fee incurred by the users

At this point, attacker balances are as follows (2,000,000 USDC in total)

Attacker's wallet in Ethereum = 1,000,000 USDC

Attacker's wallet in Optimism = 0 oUSDC

Attacker's router in Optimism = 1,000,000 oUSDC

First, the attacker attempts to transfer an extremely large amount - 1,000,000 USDC from attacker's address in Ethereum to attacker's address in Optimism Chain. The transfer amount should be much larger than the rest of the router's liquidity so that only attacker's router is capable of providing the liqudity.

In our example, since Router B does not have sufficient liquidity to faciliate the fast transfer, Router B will not be selected by the Sequencer. Since only Router A has sufficient liquidity, Router A, which is owned by the attacker, will faciliate the fast transfer and selected by the Sequencer.

Since the liquidity fee is 5%, Router A only need to supply 950,000 oUSDC on execute. The Sponsor will then reimburse 50% of the liquidity fee, which is25,000 oUSDC in total. The final amount of oUSDC send to the attacker's wallet address in Optimism will be 975,000 oUSDC.

At this point, attacker balances are as follows (1,025,000 USDC in total)

Attacker's wallet in Ethereum = 0 USDC

Attacker's wallet in Optimism = 975,000 oUSDC

Attacker's router in Optimism = 50,000 oUSDC

When the nomad message arrives, the attacker will be reimbursed 1,000,000 oUSDC when BridgeFacet._reconcile is triggered.

At this point, attacker balances are as follows (2,025,000 USDC in total)

Attacker's wallet in Ethereum = 0 USDC

Attacker's wallet in Optimism = 975,000 oUSDC

Attacker's router in Optimism = 50,000 + 1,000,000 oUSDC

Attacker earned 25,000 USDC, and SponsorVault lost 25,000 USDC.

Impact

Router owner can intentionally perform many large transfer between their own wallets in two different domain to siphon all the funds from the SponsorVault, and then proceed to withdraw all liquidity from his router.

Recommended Mitigation Steps

Although having a sponsor to subside the liqudity fee to encourage users to use sponsor's chain, this subsidy can be gamed by malicious actors for their own benefits. It is recommended to reconsider the need of having a sponsor in Connext as extreme care have to be taken in its design to ensure that it will not be exploited.

Lines of code

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

Vulnerability details

Background

AAVE Portal

AAVE portal provides a trusted credit line that allows bridges to take on an unbacked position, and Connext intents to use this credit line to provide fast-liquidity for its users in the event the routers do not have sufficient liquidity.

Connext will assign one (1) router to be responsible for taking on credit risk of borrowing an unbacked position from AAVE portal as per Source Code

Under normal circumstance, the BridgeFacet._reconcile function will automatically repay back the loan to AAVE portal when the nomad message arrives. However, if the repayment fails for certain reason, Connext expects that the router will use the repayAavePortal function out-of-band to help Connext to repay the loan.

Ultimately, it is Connext that take on the credit risk because AAVE portal only provides a trusted credit line to Connext, but not to the individual routers.

Nomad Message

When nomad message arrives, it will call BridgeFacet.handle function, which will in turn trigger the internal _reconcile function. Note that the handle or _reconcile function cannot be reverted under any circumstances because nomad message cannot be reprocessed on the nomad side.

Proof-of-Concept

1. Alice transfers 1,000,000 DAI from Ethereum domain to Polygon domain

2. None of the existing routers have sufficient liquidity, thus the sequencer decided that AAVE Portal should be used

3. Bob's router has been selected to take on the credit risk for the unbacked position, and Connext proceeds to borrow 1,000,000 DAI from AAVE Portal and send the 1,000,000 DAI to Alice's wallet on Polygon domain

4. When slow nomad message arrives, BridgeFacet._reconcile function is triggered to attempt to repay back the loan to AAVE portal. This function will in turn trigger the BridgeFacet._reconcileProcessPortal function where the portal repayment logic resides.

5. Within the BridgeFacet._reconcileProcessPortal, notice that if the AssetLogic.swapFromLocalAssetIfNeededForExactOut swap fails, it will return _amount.

6. Within the BridgeFacet._reconcileProcessPortal, notice that if the ``AavaPool.backUnbackedexternal repayment call fails, it will setamountIn = 0 , and then return [return (_amount - amountIn)](https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L1061) , which is basically the same as _amount`.

7. When the _reconcileProcessPortal function call returned at Line 603, it will set the toDistribute to the amount. amount in this example is 1,000,000 DAI.

8. Next, at Line 611, the contract will increase Bob's router balance by 1,000,000 DAI

9. Bob notices that his router balance has increased by 1,000,000 DAI, and he could not resist the tempation of 1,000,000 DAI. Therefore, instead of helping Connext to repay the loan via repayAavePortal function out-of-band, he decided to quickly withdraws all his liquidty from his router.

10. Bob gained 1,000,000 DAI, while Connext still owns AAVE portal 1,000,000 DAI

Impact

If routers decided not to repay the loan, Connext will incur large amount of debt from AAVE portal.

Recommended Mitigation Steps

Understood that there is a whitelist for routers that can use portals to ensure that only trusted routers could use this feature. In this case, the trust entirely depends on the integrity of the router owner and the assumption that the owner will not act against Connext and its community. However, as seen in many of the past security incidents, trusted actor or even own protocol team member might turn rogue when dealing with significant gain. In the above example, 1,000,000 DAI. It is common to see even larger amount of funds transferring across the bridge.

Therefore, to overcome the above-mentioned risk, some protocol would implement m-of-n multisig or validation, which help to mitigate the risk of a single trusted actor from turning rogue and perform malicious action.

Therefore, it is recommended to reconsider such design and explore other alternatives. One such alternative would be as follows:

Assuming that the AAVE portal interest rate is fixed, therefore, the amount of repayment is deterministic, and Connext can compute the amount of repayment that needs to be repaid at any point of time.

When the BridgeFacet._reconcileProcessPortal swap or AavaPool.backUnbacked fails, do not immediately credit the Bob's router balance. Instead, escrow the amount (1,000,000 DAI) received from nomad in an Escrow contract. Implement a function called settleAAVEPortalLoan within the Escrow contract, which contains the logic to perform the necessary actions to repay AAVE portal loan. In this case, Bob is responsible for triggering the Escrow.settleAAVEPortalLoan to kick start the out-of-band repayment process. If the repayment is sucessful, Bob's router will be credited with the earning for taking on credit risk.

One positive side effect of this approach is that Bob will be incentivize to make the repayment as fast as possible because the longer he delays, the higher the interest rate, and thus less earning for him.

This approach is quite similar to the withdrawal pattern.

Lines of code

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

Vulnerability details

Proof-of-Concept

This issue is only applicable for fast-transfer. Slow transfer would not have this issue because of the built-in fraud-proof mechanism in Nomad.

First, the attacker will attempt to use Connext to send 1000 USDC from Ethereum domain to Optimism domain.

Assume that the attacker happens to be a relayer on the relayer network utilised by Connext, and the attacker's relayer happens to be tasked to relay the above execute calldata to the Optimism's Connext BridgeFacet.execute function.

Optimism's Connext BridgeFacet.execute received the execute calldata and observed within the calldata that it is a fast-transfer and Router A is responsible for providing the liquidity. It will then check that the router signature is valid, and proceed to transfer 1000 oUSDC to attacker wallet (0x123456) in Optimism.

Next, attacker will update the ExecuteArgs.local within the execute calldata to a valid local representation of canonical token (USDC) used within Polygon. Attacker will then send the modified execute calldata to Polygon's Connext BridgeFacet.execute function. Assume that the same Router A is also providing liquidity in Polygon. The BridgeFacet.execute function checks that the router signature is valid, and proceed to transfer 1000 POS-USDC to atttack wallet (0x123456) in Polygon.

At this point, the attacker has 1000 oUSDC and 1000 POS-USDC in his wallets. When the nomad message arrives at Optimism, Router A can claim the 1000 oUSDC back from Connext. However, Router A is not able to claim back any fund in Polygon.

Note that same wallet address exists on different chains. For instance, the wallet address on Etherum and Polygon is the same.

Why changing the ExecuteArgs.local does not affect the router signature verification?

This is because the router signature is generated from the transferId + pathLength only, and these data are stored within the CallParams params within the ExecuteArgs struct.

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L77

struct ExecuteArgs {
  CallParams params;
  address local; // local representation of canonical token
  address[] routers;
  bytes[] routerSignatures;
  uint256 amount;
  uint256 nonce;
  address originSender;
}

Within the BridgeFacet._executeSanityChecks function, it will attempt to rebuild to transferId by calling the following code:

// Derive transfer ID based on given arguments.
bytes32 transferId = _getTransferId(_args);

Within the BridgeFacet._getTransferId function, we can see that the s.tokenRegistry.getTokenId(_args.local) will always return the canonical tokenDomain and tokenId. In our example, it will be Ethereum and USDC. Therefore, as long as the attacker specify a valid local representation of canonical token on a chain, the transferId returned by s.tokenRegistry.getTokenId(_args.local) will always be the same across all domains. Thus, this allows the attacker to modify the ExecuteArgs.local and yet he could pass the router signature check.

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

  function _getTransferId(ExecuteArgs calldata _args) private view returns (bytes32) {
    (uint32 tokenDomain, bytes32 tokenId) = s.tokenRegistry.getTokenId(_args.local);
    return _calculateTransferId(_args.params, _args.amount, _args.nonce, tokenId, tokenDomain, _args.originSender);
  }

Impact

Router liquidity would be drained by attacker, and affected router owner could not claim back their liquidity.

Recommended Mitigation Steps

The security of the current Connext design depends on how secure or reliable the relayer is. If the relayer turns rouge or act against Connext, many serious consequences can happen.

The root cause is that the current design places enormous trust on the relayers to accurately and reliably to deliver calldata to the bridge in various domains. For instance, delivering of execute call data to execute function. There is an attempt to prevent message replay on a single domain, however, it does not prevent message replay across multiple domains. Most importantly, the Connext's bridge appears to have full trust on the calldata delivered by the relayer. However, the fact is that the calldata can always be altered by the relayer.

Consider a classic 0x off-chain ordering book protocol. A user will sign his order with his private key, and attach the signature to the order, and send the order (with signature) to the relayer network. If the relayer attempts to tamper the order message or signature, the decoded address will be different from the signer's address and this will be detected by 0x's Smart contract on-chain when processing the order. This ensures that the integrity of the message and signer can be enforced.

Per good security practice, relayer network should always be considered as a hostile environment/network. Therefore, it is recommended that similar approach could be taken with regards to passing execute calldata across domains/chains.

For instance, at a high level, the sequencer should sign the execute calldata with its private key, and attach the signature to the execute calldata. Then, submit the execute calldata (with signature) to the relayer network. When the bridge receives the execute calldata (with signature), it can verify if the decoded address matches the sequencer address to ensure that the calldata has not been altered. This will ensure the intergrity of the execute calldata and prevent any issue that arise due to unauthorised modification of calldata.

Additionally, the execute calldata should also have a field that correspond to the destination domain. The bridge that receives the execute calldata must verify that the execute calldata is intended for its domain, otherwise reject the calldata if it belongs to other domains. This also helps to prevent the attack mentioned earlier where same execute calldata can be accepted in different domains.

Lines of code

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

Vulnerability details

Proof-of-Concept

Connext relies on the relayer to trigger the BridgeFacet.execute function on the destination domain to initiate the token transfer and calldata execution processes. Relayers pay for the gas cost to trigger the execute function, and in return for their effort, they are reimbused with the relayer fee.

However, it is possible that the BridgeFacet.execute function will revert under certain circumstances when triggered by the relayers. For instance, when the BridgeFacet.execute function is triggered, it will call the BridgeFacet._handleExecuteLiquidity function. Within the BridgeFacet._handleExecuteLiquidity function, it will attempt to perform token swap using a StablePool. If the slippage during the swap exceeded the user-defined value, the swap will revert and subseqently the execute will revert too.

When the BridgeFacet.execute reverts, the relayers will not receive any relayer fee.

The following code shows that the relayer who can claim the relayer fee is set within the BridgeFacet.execute function at Line 415. Therefore, if this function reverts, relayer will not be able to claim the fee.

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

  function execute(ExecuteArgs calldata _args) external whenNotPaused nonReentrant returns (bytes32) {
    (bytes32 transferId, bool reconciled) = _executeSanityChecks(_args);

    // Set the relayer for this transaction to allow for future claim
    s.transferRelayer[transferId] = msg.sender;

    // execute router liquidity when this is a fast transfer
    // asset will be adopted unless specified to be local in params
    (uint256 amount, address asset) = _handleExecuteLiquidity(transferId, !reconciled, _args);

    // execute the transaction
    uint256 amountWithSponsors = _handleExecuteTransaction(_args, amount, asset, transferId, reconciled);

    // emit event
    emit Executed(transferId, _args.params.to, _args, asset, amountWithSponsors, msg.sender);

    return transferId;
  }

Impact

Lost of fund for the relayers as they pay for the gas cost to trigger the functions, but did not receive any relayer fee in return.

Recommended Mitigation Steps

Update the implementation of the BridgeFacet.execute so that it will fail gracefully and not revert when the swap fails or other functions fails. Relayers should be entitled to relayer fee regardless of the outcome of the BridgeFacet.execute call for their effort.

Lines of code

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

Vulnerability details

Proof-of-Concept

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. For example Tether (USDT)'s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals.

Instance 1 - BridgeFacet._reconcileProcessPortal

The following function must be approved by zero first, and then the SafeERC20.safeIncreaseAllowance function can be called. Otherwise, the _reconcileProcessPortal function will revert everytime it handles such kind of tokens. Understood from the comment that after the backUnbacked call there could be a remaining allowance.

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

  function _reconcileProcessPortal(
    uint256 _amount,
    address _local,
    address _router,
    bytes32 _transferId
  ) private returns (uint256) {
  	..SNIP..
    SafeERC20.safeIncreaseAllowance(IERC20(adopted), s.aavePool, totalRepayAmount);

    (bool success, ) = s.aavePool.call(
      abi.encodeWithSelector(IAavePool.backUnbacked.selector, adopted, backUnbackedAmount, portalFee)
    );
  	..SNIP..
  }

Instance 2 - BridgeFacet_swapAssetOut

The following fucntion must first be approved by zero, follow by the actual allowance to be approved. Otherwise, the _swapAssetOut function will revert everytime it handles such kind of tokens.

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/AssetLogic.sol#L347

  function _swapAssetOut(
    bytes32 _canonicalId,
    address _assetIn,
    address _assetOut,
    uint256 _amountOut,
    uint256 _maxIn
  )
    internal
    returns (
      bool,
      uint256,
      address
    )
  {
    AppStorage storage s = LibConnextStorage.connextStorage();

    bool success;
    uint256 amountIn;

    // Swap the asset to the proper local asset
    if (stableSwapPoolExist(_canonicalId)) {
      // get internal swap pool
      SwapUtils.Swap storage ipool = s.swapStorages[_canonicalId];
      // if internal swap pool exists
      uint8 tokenIndexIn = getTokenIndexFromStableSwapPool(_canonicalId, _assetIn);
      uint8 tokenIndexOut = getTokenIndexFromStableSwapPool(_canonicalId, _assetOut);
      // calculate slippage before performing swap
      // NOTE: this is less efficient then relying on the `swapInternalOut` revert, but makes it easier
      // to handle slippage failures (this can be called during reconcile, so must not fail)
      if (_maxIn >= ipool.calculateSwapInv(tokenIndexIn, tokenIndexOut, _amountOut)) {
        success = true;
        amountIn = ipool.swapInternalOut(tokenIndexIn, tokenIndexOut, _amountOut, _maxIn);
      }
      // slippage is too high to perform swap: success = false, amountIn = 0
    } else {
      // Otherwise, swap via stable swap pool
      IStableSwap pool = s.adoptedToLocalPools[_canonicalId];
      uint256 _amountIn = pool.calculateSwapOutFromAddress(_assetIn, _assetOut, _amountOut);
      if (_amountIn <= _maxIn) {
        // set the success
        success = true;

        // perform the swap
        SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);
        amountIn = pool.swapExactOut(_amountOut, _assetIn, _assetOut, _maxIn);
      }
      // slippage is too high to perform swap: success = false, amountIn = 0
    }

    return (success, amountIn, _assetOut);
  }

Impact

Both the_reconcileProcessPortal and _swapAssetOut functions are called during repayment to Aave Portal if the fast-transfer was executed using portal liquidity. Thus, it is core part of the token transfer process within Connext, and failure of any of these functions would disrupt the AAVE repayment process.

Since both functions affect the AAVE repayment process, I'm grouping them as one issue.

Recommended Mitigation Steps

As Connext bridges/routers deal with all sort of tokens existed in various domains/chains, the protocol should try to implement measure to ensure that it is compatible with as much tokens as possible for future growth and availability of the protocol.

Instance 1 - BridgeFacet._reconcileProcessPortal

It is recommended to set the allowance to zero before increasing the allowance

SafeERC20.safeApprove(IERC20(_assetIn), address(pool), 0);
SafeERC20.safeIncreaseAllowance(IERC20(adopted), s.aavePool, totalRepayAmount);

Instance 2 - BridgeFacet_swapAssetOut

It is recommended to set the allowance to zero before each approve call.

SafeERC20.safeApprove(IERC20(_assetIn), address(pool), 0);
SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);

Lines of code

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

Vulnerability details

Proof-of-Concept

Assume this is a fast-transfer path and the sequencer has a good reason (e.g. some sophisticated liquidity load balancing algorithm) to assign 3 routers to provide liquidity for a transfer of 90 DAI

Therefore, each of them will provide 30 DAI equally.

_args.routers[] array = [Router A, Router B, Router C]

However, a malicious relayer could rearrange the _args.routers[] array to any of the following and still pass the sanity checks within BridgeFacet._executeSanityChecks

_args.routers[] array = [Router A, Router A, Router A]

_args.routers[] array = [Router A, Router A, Router B]

_args.routers[] array = [Router A, Router A, Router C]

_args.routers[] array = [Router C, Router A, Router C]

The point is that as long as the attacker ensures that the pathLength is correct, he will be able to pass the router check within BridgeFacet._executeSanityChecks.

Assume that malicious relayer decided to rearrange the _args.routers[] array to as follows, this will cause Router A to provide more liquidity than it should be and overwrite the sequencer decision. In this case, Router A will be forced to provide 90 DAI.

_args.routers[] array = [Router A, Router A, Router A]

This is possible because the routerHash used to generate the router signature only consists of two items (transferId and pathLength)

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

  function _executeSanityChecks(ExecuteArgs calldata _args) private view returns (bytes32, bool) {
    ..SNIP..
    // Derive transfer ID based on given arguments.
    bytes32 transferId = _getTransferId(_args);

    // Retrieve the reconciled record. If the transfer is `forceSlow` then it must be reconciled first
    // before it's executed.
    bool reconciled = s.reconciledTransfers[transferId];
    if (_args.params.forceSlow && !reconciled) revert BridgeFacet__execute_notReconciled();

    // Hash the payload for which each router should have produced a signature.
    // Each router should have signed the `transferId` (which implicitly signs call params,
    // amount, and tokenId) as well as the `pathLength`, or the number of routers with which
    // they are splitting liquidity provision.
    bytes32 routerHash = keccak256(abi.encode(transferId, pathLength));

    // check the reconciled status is correct
    // (i.e. if there are routers provided, the transfer must *not* be reconciled)
    if (pathLength > 0) // make sure routers are all approved if needed
    {
      if (reconciled) revert BridgeFacet__execute_alreadyReconciled();

      for (uint256 i; i < pathLength; ) {
        // Make sure the router is approved, if applicable.
        // If router ownership is renounced (_RouterOwnershipRenounced() is true), then the router whitelist
        // no longer applies and we can skip this approval step.
        if (!_isRouterOwnershipRenounced() && !s.routerPermissionInfo.approvedRouters[_args.routers[i]]) {
          revert BridgeFacet__execute_notSupportedRouter();
        }

        // Validate the signature. We'll recover the signer's address using the expected payload and basic ECDSA
        // signature scheme recovery. The address for each signature must match the router's address.
        if (_args.routers[i] != _recoverSignature(routerHash, _args.routerSignatures[i])) {
          revert BridgeFacet__execute_invalidRouterSignature();
        }

        unchecked {
          i++;
        }
      }
    ..SNIP..
  }

Impact

Malicious relayer could overwrite sequencer decision, or cause a certain router to drain more liquidity than it should be.

Recommended Mitigation Steps

Generate the routerHash with the following items should help to prevent this attack:

• transferId

• pathLength

• _args.routers[] array

In this case, if the attacker attempts to re-arrange the _args.routers[] array, the routerHash generate on the bridge will be different. Thus, it will fail the router signature verification within _executeSanityChecks function and it will revert.

Lines of code

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

Vulnerability details

Vulnerability Details

Assume that a malicious relayer operates a router in Connext providing fast-liquidity service. A malicious relayer could always swap the router(s) within the execute calldata with the router(s) owned by malicious relayer, and submit it to the chain for execution.

Proof-of-Concept

This example assumes a fast-liquidity path. When the relayer posts a execute calldata to destination domain's BridgeFacet.execute function, this function will trigger the BridgeFacet._executeSanityChecks function to perform a sanity check.

Assume that the execute calldata only have 1 router selected. A malicious relayer could perform the following actions:

1. Attacker removes original router from _args.routers[] array, and remove original router's signature from _args.routerSignatures[] array from the execute calldata.
2. Attacker re-calculates the routerHash (routerHash = keccak256(abi.encode(transferId, pathLength))). pathLength = 1 in this example
3. Attacker signs the routerHash with his own router's private key to obtain the router signature
4. Attacker inserts his router to the _args.routers[] array, and insert the router signature obtained fromthe previous step to _args.routerSignatures[] array
5. Submit the modified execute calldata to destination domain's BridgeFacet.execute function, and it will pass the BridgeFacet._executeSanityChecks function since router signature is valid.

The BridgeFacet._executeSanityChecks function is not aware of the fact that the router within the execute calldata has been changed because it will only check if the router specified within the _args.routers[] array matches with the router signature provided. Once the sanity check passes, it will store the attacker's router within s.routedTransfers[_transferId] and proceed with providing fast-liquidity service for the users.

The existing mechanism is useful in preventing malicious relayer from specifying routers belonging to someone else because the malicious relayer would not be capable of generating a valid router signature on behalf of other routers because he does not own their private key. However, this mechanism does not guard against a malicious relayer from specifying their own router because in this case they would be able to generate a valid router signature as they own the private key.

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

 /**
   * @notice Performs some sanity checks for `execute`
   * @dev Need this to prevent stack too deep
   */
  function _executeSanityChecks(ExecuteArgs calldata _args) private view returns (bytes32, bool) {
    // If the sender is not approved relayer, revert
    if (!s.approvedRelayers[msg.sender] && msg.sender != _args.params.agent) {
      revert BridgeFacet__execute_unapprovedSender();
    }

    // Path length refers to the number of facilitating routers. A transfer is considered 'multipath'
    // if multiple routers provide liquidity (in even 'shares') for it.
    uint256 pathLength = _args.routers.length;

    // Make sure number of routers is below the configured maximum.
    if (pathLength > s.maxRoutersPerTransfer) revert BridgeFacet__execute_maxRoutersExceeded();

    // Derive transfer ID based on given arguments.
    bytes32 transferId = _getTransferId(_args);

    // Retrieve the reconciled record. If the transfer is `forceSlow` then it must be reconciled first
    // before it's executed.
    bool reconciled = s.reconciledTransfers[transferId];
    if (_args.params.forceSlow && !reconciled) revert BridgeFacet__execute_notReconciled();

    // Hash the payload for which each router should have produced a signature.
    // Each router should have signed the `transferId` (which implicitly signs call params,
    // amount, and tokenId) as well as the `pathLength`, or the number of routers with which
    // they are splitting liquidity provision.
    bytes32 routerHash = keccak256(abi.encode(transferId, pathLength));

    // check the reconciled status is correct
    // (i.e. if there are routers provided, the transfer must *not* be reconciled)
    if (pathLength > 0) // make sure routers are all approved if needed
    {
      if (reconciled) revert BridgeFacet__execute_alreadyReconciled();

      for (uint256 i; i < pathLength; ) {
        // Make sure the router is approved, if applicable.
        // If router ownership is renounced (_RouterOwnershipRenounced() is true), then the router whitelist
        // no longer applies and we can skip this approval step.
        if (!_isRouterOwnershipRenounced() && !s.routerPermissionInfo.approvedRouters[_args.routers[i]]) {
          revert BridgeFacet__execute_notSupportedRouter();
        }

        // Validate the signature. We'll recover the signer's address using the expected payload and basic ECDSA
        // signature scheme recovery. The address for each signature must match the router's address.
        if (_args.routers[i] != _recoverSignature(routerHash, _args.routerSignatures[i])) {
          revert BridgeFacet__execute_invalidRouterSignature();
        }

        unchecked {
          i++;
        }
      }
    ..SNIP..
  }

When the nomad message eventually reaches the destination domain and triggered to the BridgeFacet._reconcile, the attacker's router will be able to claim back the asset provided in the execution step as per normal.

Impact

Malicious relayer could force Connext to use those routers owned by them to earn the liquidity fee, and at the same time causes the original router chosen by the sequencer to lost the opportunity to earn the liquidity fee. This disrupts the balance and fairness of the protocol causing normal routers to lost the opportunity to earn liquidity fee.

In bridge or cross-chain communication design, it is a good security practice to minimize the trust that Connext places on other external protocol (e.g. relayer network) wherever possible so that if the external protocol is compromised or acting against maliciously against Connext, the impact or damage would be reduced.

Recommended Mitigation Steps

It is recommended to devise a way for the Connext's destination bridge to verify that the execute calldata received from the relayer is valid and has not been altered. Ideally, the hash of the original execute calldata sent by seqencer should be compared with the hash of the execute calldata received from relayer so that a mismatch would indicate that the calldata has been modified along the way, and some action should be taken.

For instance, consider a classic 0x off-chain ordering book protocol. A user will sign his order with his private key, and attach the signature to the order, and send the order (with signature) to the relayer network. If the relayer attempts to tamper the order message or signature, the decoded address will be different from the signer's address and this will be detected by 0x's Smart contract on-chain when processing the order. This ensures that the integrity of the message and signer can be enforced.

Per good security practice, relayer network should always be considered as a hostile environment/network. Therefore, it is recommended that similar approach could be taken with regards to passing execute calldata across domains/chains.

For instance, at a high level, the sequencer should sign the execute calldata with its private key, and attach the signature to the execute calldata. Then, submit the execute calldata (with signature) to the relayer network. When the bridge receives the execute calldata (with signature), it can verify if the decoded address matches the sequencer address to ensure that the calldata has not been altered. This will ensure the intergrity of the execute calldata and prevent any issue that arise due to unauthorised modification of calldata.

Alternative Solution

Alternatively, following method could also be adopted to prevent this issue:

1. Assume that it is possible to embed the selected router(s) within the slow nomad message. Append the selected router(s) within the slow nomad message

2. Within the BridgeFacet.execute function, instead of using only the transfer ID as the array index (s.routedTransfers[_transferId] = _args.routers; ), use both transfer ID + selected router as the array index (s.routedTransfers[hash(_transferId+routers)] = _args.routers;)

3. When the slow nomad message arrives and triggers to the BridgeFacet._reconcile, this function will find the routers that provide the fast-liquidity based on the information within the nomad message only. It will attempt to call s.routedTransfers[hash(_transferId+routers)] and it should return nothing as there is a mismatch between attacker's router and router within the nomad message.

4. In this case, the attacker will not be able to claim back any of the funds he provided earlier. This will deter anyone from attempting to swap the router within the execute calldata sent to destination domain's BridgeFacet.execute function because they will not be able to claim back the funds

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/RoutersFacet.sol#L293 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/RoutersFacet.sol#L490 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/RoutersFacet.sol#L212

Vulnerability details

Proof-of-Concept

Assume that Alice's router has large amount of liquidity inside.

Assume that the Connext Admin decided to remove a router owned by Alice. The Connext Admin will call the RoutersFacet.removeRouter function, and all information related to Alice's router will be erased (set to 0x0) from the s.routerPermissionInfo.

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

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

Alice is aware that her router has been removed by Connext Admin, so she decided to withdraw the liquidity from her previous router by calling RoutersFacet.removeRouterLiquidityFor.

However, when Alice called the RoutersFacet.removeRouterLiquidityFor function, it will revert every single time. This is because the condition msg.sender != getRouterOwner(_router) will always fail.

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

  /**
   * @notice This is used by any router owner to decrease their available liquidity for a given asset.
   * @param _amount - The amount of liquidity to remove for the router
   * @param _local - The address of the asset you're removing liquidity from. If removing liquidity of the
   * native asset, routers may use `address(0)` or the wrapped asset
   * @param _to The address that will receive the liquidity being removed
   * @param _router The address of the router
   */
  function removeRouterLiquidityFor(
    uint256 _amount,
    address _local,
    address payable _to,
    address _router
  ) external nonReentrant whenNotPaused {
    // Caller must be the router owner
    if (msg.sender != getRouterOwner(_router)) revert RoutersFacet__removeRouterLiquidityFor_notOwner();

    // Remove liquidity
    _removeLiquidityForRouter(_amount, _local, _to, _router);
  }

Since the RoutersFacet.removeRouter function has earlier erased all information related to Alice's router within s.routerPermissionInfo, the getRouterOwner function will always return the router address.

In this case, the router address will not match against msg.sender address/Alice address, thus Alice attempts to call removeRouterLiquidityFor will always revert.

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

  function getRouterOwner(address _router) public view returns (address) {
    address _owner = s.routerPermissionInfo.routerOwners[_router];
    return _owner == address(0) ? _router : _owner;
  }

Impact

Router owner who provides liquidity could be rugged by Connext admin. When this happen, the router owner funds will be struck within the RoutersFacet contract, and there is no way for the router owner to retrieve their liquidity.

In the worst case scenario, a compromised Connext admin could remove all routers, and cause all liquidity to be struck within RoutersFacet and no router owner could withdraw their liquidity from the contract. Next, the RouterFacet contract could be upgraded to include additional function to withdraw all liquidity from the contract to an arbitrary wallet address.

Recommended Mitigation Steps

The router owner is still entitled to their own liquidity even though their router has been removed by Connext Admin. Thus, they should be given the right to take back their liquidity when such an event happens. The contract should update its implementation to support this. This will give more assurance to the router owner.

Lines of code

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

Vulnerability details

Proof-of-Concept

A third party sponsor would need to implement a SponsorVault contract that is aligned with the ISponsorVault interface.

Assume that a SponsorVault contract has been defined on Optimism chain. All cross-chain communications are required to call the BridgeFacet.execute, which in turn will trigger the BridgeFacet._handleExecuteTransaction internal function.

However, if there is an error within SponsorVault contract in Optimism causing a revert when s.sponsorVault.reimburseLiquidityFees or s.sponsorVault.reimburseRelayerFees is called, the entire execute transaction will revert. Since execute transaction always revert, any cross-chain communication between Optimism and other domains will fail.

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

  /**
   * @notice Process the transfer, and calldata if needed, when calling `execute`
   * @dev Need this to prevent stack too deep
   */
  function _handleExecuteTransaction(
    ExecuteArgs calldata _args,
    uint256 _amount,
    address _asset, // adopted (or local if specified)
    bytes32 _transferId,
    bool _reconciled
  ) private returns (uint256) {
    // If the domain if sponsored
    if (address(s.sponsorVault) != address(0)) {
      // fast liquidity path
      if (!_reconciled) {
        // Vault will return the amount of the fee they sponsored in the native fee
        // NOTE: some considerations here around fee on transfer tokens and ensuring
        // there are no malicious `Vaults` that do not transfer the correct amount. Should likely do a
        // balance read about it

        uint256 starting = IERC20(_asset).balanceOf(address(this));
        uint256 sponsored = s.sponsorVault.reimburseLiquidityFees(_asset, _args.amount, _args.params.to);

        // Validate correct amounts are transferred
        if (IERC20(_asset).balanceOf(address(this)) != starting + sponsored) {
          revert BridgeFacet__handleExecuteTransaction_invalidSponsoredAmount();
        }

        _amount = _amount + sponsored;
      }

      // Should dust the recipient with the lesser of a vault-defined cap or the converted relayer fee
      // If there is no conversion available (i.e. no oracles for origin domain asset <> dest asset pair),
      // then the vault should just pay out the configured constant
      s.sponsorVault.reimburseRelayerFees(_args.params.originDomain, payable(_args.params.to), _args.params.relayerFee);
    }
    ..SNIP..

Impact

It will result in denial of service. The SponsorVault contract, which belongs to a third-party, is a single point of failure for a domain.

Recommended Mitigation Steps

This is a problem commonly encountered whenever a method of a smart contract calls another contract – we cannot rely on the other contract to work 100% of the time, and it is dangerous to assume that the external call will always be successful. Additionally, external smart contract might be vulnerable and compromised by an attacker. Even if the team has audited or review the SponsorVault before whitelisting them, some risk might still exist.

Therefore, it is recommended to implement a fail-safe design where failure of an external call to SponsorVault will not disrupt the cross-chain communication. Consider implementing a try-catch block as shown below. If there is any issue with the external SponsorVault contract, no funds are reimbursed to the users in the worst case scenario, but the issue will not cause any impact to the cross-chain communication.

function _handleExecuteTransaction(
	ExecuteArgs calldata _args,
	uint256 _amount,
	address _asset, // adopted (or local if specified)
	bytes32 _transferId,
	bool _reconciled
) private returns (uint256) {
	// If the domain if sponsored
	if (address(s.sponsorVault) != address(0)) {
	  // fast liquidity path
	  if (!_reconciled) {
		// Vault will return the amount of the fee they sponsored in the native fee
		// NOTE: some considerations here around fee on transfer tokens and ensuring
		// there are no malicious `Vaults` that do not transfer the correct amount. Should likely do a
		// balance read about it

		uint256 starting = IERC20(_asset).balanceOf(address(this));
+		try s.sponsorVault.reimburseLiquidityFees(_asset, _args.amount, _args.params.to) returns (uint256 sponsored) {
+			// Validate correct amounts are transferred
+			if (IERC20(_asset).balanceOf(address(this)) != starting + sponsored) {
+			  revert BridgeFacet__handleExecuteTransaction_invalidSponsoredAmount();
+			}
+
+			_amount = _amount + sponsored;
+		} catch {}
	  }

	  // Should dust the recipient with the lesser of a vault-defined cap or the converted relayer fee
	  // If there is no conversion available (i.e. no oracles for origin domain asset <> dest asset pair),
	  // then the vault should just pay out the configured constant
+	  try s.sponsorVault.reimburseRelayerFees(_args.params.originDomain, payable(_args.params.to), _args.params.relayerFee) {} catch {}
	..SNIP..

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

Unused Function and Parameter

Proof-of-Concept

Following function is not used anywhere in the contracts.

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

  function _reconcileProcessMessage(bytes memory _message)
    internal
    returns (
      uint256,
      address,
      bytes32
    )
  {

The _router parameter is not used within the repayAavePortalFor function. Consider removing it if it is not required.

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

  function repayAavePortalFor(
    address _router,
    address _adopted,
    uint256 _backingAmount,
    uint256 _feeAmount,
    bytes32 _transferId
  ) external payable {

Recommended Mitigation Steps

Consider removing it if it is not required.

No two-step process for ownership transfer

Proof-of-Concept

The current admin transfer process involves the current admin calling ConnextPriceOracle.setAdmin function. If the nominated EOA account is not a valid account, it is entirely possible the admin might be accidentally transferred to an uncontrolled account.

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L168

  function setAdmin(address newAdmin) external onlyAdmin {
    address oldAdmin = admin;
    admin = newAdmin;

    emit NewAdmin(oldAdmin, newAdmin);
  }

Recommended Mitigation Steps

Consider implementing a two step process where the admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of admin to fully succeed. This ensures the nominated EOA account is a valid and active account.

SafeApprove Has Been Deprecated

Proof-of-Concept

OpenZeppelin SafeERC20 safeApprove() function was found to be used in a number of contracts. This function has been deprecated. Refer to the comments in the SafeERC20's source code (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/828fe365eeff13e7aa188e449005ad81f7222189/contracts/token/ERC20/utils/SafeERC20.sol#L39-L44)

An example of the usage of safeApprove() is shown below:

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/AssetLogic.sol#L347

Recommended Mitigation Steps

Use safeIncreaseAllowance() instead of safeApprove()

Todo

Proof-of-Concept

A number of TODO are found in the comments within the source code.

https://github.com/connext/nxtp/blob/04d081b4fca22eba27976a2f6e5fdd4e7b8f17a5/packages/deployments/contracts/contracts/nomad-core/contracts/upgrade/UpgradeBeaconController.sol#L17

// TODO: upgrade to `ProposedOwnable`
contract UpgradeBeaconController is Ownable {

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

// TODO: do we want to store a mapping of custodied token balances here?

// Token is local for this domain. We should custody the token here.
// Query token contract for details and calculate detailsHash.
detailsHash = ConnextMessage.formatDetailsHash(token.name(), token.symbol(), token.decimals());

Recommended Mitigation Steps

It is recommended to review them to ensure that they are resolved before deploying the contracts to the Production environment (e.g. Mainnet)

Add constructor initializer in implementation contracts

Proof-of-Concept

It is a good practice to an empty constructor with the initializer modifier to the RelayerFeeRouter and PromiseRouter contracts. So the implementation contract gets initialized automatically upon deployment. This is to prevent attack similar to https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/6

Affected Contracts

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol#L80

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L146

Residual Allowance Left

Proof-of-Concept

The AssetLogic._swapAssetOut function authorised the StableSwap pool contract to spend/draw _amountIn amount of tokens from the bridge for the swap.

SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);

This function will attempt to swap exactly _amountOut number of tokens, by spending up to _maxIn.

The amount of tokens that was transfer to this contract is _amountIn. However, during the swap process, it is likely that the amount of tokens utilsed for the swap is less than _amountIn. Thus, there will be unnecessary residual allowance left after the swap.

  function _swapAssetOut(
    bytes32 _canonicalId,
    address _assetIn,
    address _assetOut,
    uint256 _amountOut,
    uint256 _maxIn
  )
    internal
    returns (
      bool,
      uint256,
      address
    )
  {
    AppStorage storage s = LibConnextStorage.connextStorage();

    bool success;
    uint256 amountIn;

    // Swap the asset to the proper local asset
    if (stableSwapPoolExist(_canonicalId)) {
      // get internal swap pool
      SwapUtils.Swap storage ipool = s.swapStorages[_canonicalId];
      // if internal swap pool exists
      uint8 tokenIndexIn = getTokenIndexFromStableSwapPool(_canonicalId, _assetIn);
      uint8 tokenIndexOut = getTokenIndexFromStableSwapPool(_canonicalId, _assetOut);
      // calculate slippage before performing swap
      // NOTE: this is less efficient then relying on the `swapInternalOut` revert, but makes it easier
      // to handle slippage failures (this can be called during reconcile, so must not fail)
      if (_maxIn >= ipool.calculateSwapInv(tokenIndexIn, tokenIndexOut, _amountOut)) {
        success = true;
        amountIn = ipool.swapInternalOut(tokenIndexIn, tokenIndexOut, _amountOut, _maxIn);
      }
      // slippage is too high to perform swap: success = false, amountIn = 0
    } else {
      // Otherwise, swap via stable swap pool
      IStableSwap pool = s.adoptedToLocalPools[_canonicalId];
      uint256 _amountIn = pool.calculateSwapOutFromAddress(_assetIn, _assetOut, _amountOut);
      if (_amountIn <= _maxIn) {
        // set the success
        success = true;

        // perform the swap
        SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);
        amountIn = pool.swapExactOut(_amountOut, _assetIn, _assetOut, _maxIn);
      }
      // slippage is too high to perform swap: success = false, amountIn = 0
    }

Recommended Mitigation Steps

Reset the allowance to zero after the swap. This will ensure that the StablePool contract will not be able to withdraw any residual allowance/funds from the bridge under any circumstances.

SafeERC20.safeApprove(IERC20(_assetIn), address(pool), 0); // Reset allowance to zero.

StablePool contract is owned by Connext, therefore marking this issue as low.

Does Not Check If Admin Is Address(0)

Proof-of-Concept

The ConnextPriceOracle.setAdmin function does not check if the new admin address provided is address(0). Thus, the ownership of the contract might be accidentally transferred to address(0).

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L168

  function setAdmin(address newAdmin) external onlyAdmin {
    address oldAdmin = admin;
    admin = newAdmin;

    emit NewAdmin(oldAdmin, newAdmin);
  }

Recommended Mitigation Steps

Implement a require check to ensure that new admin address provided is not address(0).