Connext Amarok contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L346-L349

Vulnerability details

_swapAssetOut() performs one step approval for an arbitrary _assetIn before calling pool's swapExactOut(). As ERC20 that do not allow approval race condition prohibit setting approval to a new positive value when allowance is positive already, this call will fail if _assetIn happens to be such a token.

It can happen as _swapAssetOut has to deal with arbitrary tokens and swapping can produce leftover allowance for the next runs by not utilizing current amount fully.

Setting severity to medium as that situation will render _swapAssetOut and all the upstream functionality unavailable for such a tokens: all successive calls will be reverted until allowance be used up or reset.

Proof of Concept

_swapAssetOut() approves positive _amountIn of _assetIn in one step:

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L346-L349

        // perform the swap
        SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);
        amountIn = pool.swapExactOut(_amountOut, _assetIn, _assetOut, _maxIn);
      }

Some tokens do not let to approve new positive allowance from current positive allowance to protect against race condition when the allowance can be double spent, simply reverting any attempt to approve positive from positive:

https://github.com/d-xo/weird-erc20#approval-race-protections

For example current USDT contract, L205:

https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code

Recommended Mitigation Steps

The recommendation is to set approval to zero first, then to set it to _amountIn.

It's the same as described in the another occurrence of the same issue in BridgeFacet:

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

Lines of code

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

Vulnerability details

_executePortalTransfer can introduce underlying token deficit by accounting for full underlying amount received from Aave unconditionally on what was actually withdrawn from Aave pool. Actual amount withdrawn is returned by IAavePool(s.aavePool).withdraw(), but currently is not used.

Setting the severity to medium as this can end up with a situation of partial insolvency, when where are a surplus of atokens, but deficit of underlying tokens in the bridge, so bridge functionality can become unavailable as there will be not enough underlying tokens, which were used up in the previous operations when atokens wasn't converted to underlying fully and underlying tokens from other operations were used up instead without accounting. I.e. the system in this situation supposes that all atokens are in the form of underlying tokens while there will be some atokens left unconverted due to withdrawal being only partial.

Proof of Concept

Call sequence here is execute() -> _handleExecuteLiquidity() -> _executePortalTransfer().

BridgeFacet._executePortalTransfer() mints the atokens needed, then withdraws them from Aave pool, always accounting for the full withdrawal:

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

  /**
   * @notice Uses Aave Portals to provide fast liquidity
   */
  function _executePortalTransfer(
    bytes32 _transferId,
    uint256 _fastTransferAmount,
    address _local,
    address _router
  ) internal returns (uint256, address) {
    // Calculate local to adopted swap output if needed
    (uint256 userAmount, address adopted) = AssetLogic.calculateSwapFromLocalAssetIfNeeded(_local, _fastTransferAmount);

    IAavePool(s.aavePool).mintUnbacked(adopted, userAmount, address(this), AAVE_REFERRAL_CODE);

    // Improvement: Instead of withdrawing to address(this), withdraw directly to the user or executor to save 1 transfer
    IAavePool(s.aavePool).withdraw(adopted, userAmount, address(this));

    // Store principle debt
    s.portalDebt[_transferId] = userAmount;

Aave pool's withdraw() returns the amount of underlying asset that was actually withdrawn:

https://github.com/aave/aave-v3-core/blob/master/contracts/protocol/pool/Pool.sol#L196-L217

https://github.com/aave/aave-v3-core/blob/master/contracts/protocol/libraries/logic/SupplyLogic.sol#L93-L111

If a particular lending pool has liquidity shortage at the moment, say all underlying is lent out, full withdrawal of the requested underlying token amount will not be possible.

Recommended Mitigation Steps

Consider adjusting for the amount actually withdrawn. Also the buffer that stores minted but not yet used atoken amount, say aAmountStored, can be introduced.

For example:

+   uint256 amountNeeded = userAmount < aAmountStored ? 0 : userAmount - aAmountStored;

-   IAavePool(s.aavePool).mintUnbacked(adopted, userAmount, address(this), AAVE_REFERRAL_CODE);
+   if (amountNeeded > 0) {
+       IAavePool(s.aavePool).mintUnbacked(adopted, amountNeeded, address(this), AAVE_REFERRAL_CODE);
+   }

    // Improvement: Instead of withdrawing to address(this), withdraw directly to the user or executor to save 1 transfer
-   IAavePool(s.aavePool).withdraw(adopted, userAmount, address(this));
+   uint256 amountWithdrawn = IAavePool(s.aavePool).withdraw(adopted, userAmount, address(this));

    // Store principle debt
-   s.portalDebt[_transferId] = userAmount;
+   s.portalDebt[_transferId] = amountWithdrawn; // can't exceed userAmount
+   aAmountStored = (userAmount < aAmountStored ? aAmountStored : userAmount) - amountWithdrawn; // we used amountWithdrawn

1. Unpatched token approval issue in BridgeFacet, open TODOs (low)

Proof of Concept

Race condition issue isn't addressed, open TODO:

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

    // TODO: Should we call approve(0) and approve(totalRepayAmount) instead? or with a try catch to not affect gas on all cases?
    // Example: https://github.com/aave/aave-v3-periphery/blob/ca184e5278bcbc10d28c3dbbc604041d7cfac50b/contracts/adapters/paraswap/ParaSwapRepayAdapter.sol#L138-L140
    SafeERC20.safeIncreaseAllowance(IERC20(adopted), s.aavePool, totalRepayAmount);

Another TODO comment:

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

      // TODO: do we need to keep this
      bytes32 details = action.detailsHash();
      IBridgeToken(token).setDetailsHash(details);

Recommended Mitigation Steps

Consider choosing and implementing the method to address the approval issue, i.e. either do two step 0 -> amount approval or try-catch, before release.

It's advised to remove TODO comments in either way from production version of the code.

2. Incomplete description of AssetLogic's _swapAsset function (low)

_swapAsset() function description omits _slippageTol argument:

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L252-L268

  /**
   * @notice Swaps assetIn t assetOut using the stored stable swap or internal swap pool
   * @dev Will not swap if the asset passed in is the adopted asset
   * @param _canonicalId - The canonical token id
   * @param _assetIn - The address of the from asset
   * @param _assetOut - The address of the to asset
   * @param _amount - The amount of the local asset to swap
   * @return The amount of assetOut
   * @return The address of assetOut
   */
  function _swapAsset(
    bytes32 _canonicalId,
    address _assetIn,
    address _assetOut,
    uint256 _amount,
    uint256 _slippageTol
  ) internal returns (uint256, address) {

Recommended Mitigation Steps

Add the description, for example:

  /**
   * @param _amount      - The amount of the local asset to swap
   * @param _slippageTol - Slippage tolerance with LIQUIDITY_FEE_DENOMINATOR decimals
   * @return The amount of assetOut
   * @return The address of assetOut
   */

3. Incomplete description of AssetLogic's _swapAssetOut function (low)

There is a typo and success return value is omitted:

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L294-L310

  /**
   * @notice Swaps assetIn t assetOut using the stored stable swap or internal swap pool
   * @dev Will not swap if the asset passed in is the adopted asset
   * @param _canonicalId - The canonical token id
   * @param _assetIn - The address of the from asset
   * @param _assetOut - The address of the to asset
   * @param _amountOut - The amount of the _assetOut to swap
   * @return The amount of assetIn
   * @return The address of assetOut
   */
  function _swapAssetOut(
    bytes32 _canonicalId,
    address _assetIn,
    address _assetOut,
    uint256 _amountOut,
    uint256 _maxIn
  )

Recommended Mitigation Steps

Consider updating to:

* @notice Swaps assetIn to assetOut using the stored stable swap or internal swap pool
...
* @return Swap success flag
* @return The amount of assetIn
...

4. AssetFacet's events aren't indexed (non-critical)

Filtering on unindexed events is disabled, which makes it harder to programmatically use and analyse the system.

Proof of Concept

AssetFacet's events don't have any indices:

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

  /**
   * @notice Emitted when the wrapper variable is updated
   * @param oldWrapper - The wrapper old value
   * @param newWrapper - The wrapper new value
   * @param caller - The account that called the function
   */
  event WrapperUpdated(address oldWrapper, address newWrapper, address caller);

  /**
   * @notice Emitted when the tokenRegistry variable is updated
   * @param oldTokenRegistry - The tokenRegistry old value
   * @param newTokenRegistry - The tokenRegistry new value
   * @param caller - The account that called the function
   */
  event TokenRegistryUpdated(address oldTokenRegistry, address newTokenRegistry, address caller);

  /**
   * @notice Emitted when a new stable-swap AMM is added for the local <> adopted token
   * @param canonicalId - The canonical identifier of the token the local <> adopted AMM is for
   * @param domain - The domain of the canonical token for the local <> adopted amm
   * @param swapPool - The address of the AMM
   * @param caller - The account that called the function
   */
  event StableSwapAdded(bytes32 canonicalId, uint32 domain, address swapPool, address caller);

  /**
   * @notice Emitted when a new asset is added
   * @param canonicalId - The canonical identifier of the token the local <> adopted AMM is for
   * @param domain - The domain of the canonical token for the local <> adopted amm
   * @param adoptedAsset - The address of the adopted (user-expected) asset
   * @param supportedAsset - The address of the whitelisted asset. If the native asset is to be whitelisted,
   * the address of the wrapped version will be stored
   * @param caller - The account that called the function
   */
  event AssetAdded(bytes32 canonicalId, uint32 domain, address adoptedAsset, address supportedAsset, address caller);

  /**
   * @notice Emitted when an asset is removed from whitelists
   * @param canonicalId - The canonical identifier of the token removed
   * @param caller - The account that called the function
   */
  event AssetRemoved(bytes32 canonicalId, address caller);

Recommended Mitigation Steps

Consider adding indexes to ids and addresses in the all important events to improve their usability

details is used only when !s.tokenRegistry.isLocalOrigin(token) holds:

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

    bytes32 details = action.detailsHash();

    // if the token is of remote origin, mint the tokens. will either
    // - be credited to router (fast liquidity)
    // - be reserved for execution (slow liquidity)
    if (!s.tokenRegistry.isLocalOrigin(token)) {
      IBridgeToken(token).mint(address(this), amount);
      // Tell the token what its detailsHash is
      IBridgeToken(token).setDetailsHash(details);
    }
    // NOTE: if the token is of local origin, it means it was escrowed
    // in this contract at xcall

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

    return (amount, token, transferId);
  }

Recommended Mitigation Steps

Move into the if scope:

    // if the token is of remote origin, mint the tokens. will either
    // - be credited to router (fast liquidity)
    // - be reserved for execution (slow liquidity)
    if (!s.tokenRegistry.isLocalOrigin(token)) {
      bytes32 details = action.detailsHash();
      IBridgeToken(token).mint(address(this), amount);
      // Tell the token what its detailsHash is
      IBridgeToken(token).setDetailsHash(details);
    }