code423n4 commented 2 years ago

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

The logic of reimburseLiquidityFees() is wrong and for some cases it doesn't transfer sponsoredFee to msg.sender. (when address(tokenExchanges[_token]) != address(0)) so any logic in Connext calling reimburseLiquidityFees() would be wrong too. _handleExecuteTransaction() in BridgeFacet calls it.

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 when the IF condition is True contract swaps amounts but id doesn't transfer tokens to msg.sender. the real bug is that transfer logic is written inside the else but it should be after if/else. so for tokens that address(tokenExchanges[_token]) != address(0 is true contract won't transfer sponsoredFee and any logic that uses this function would have this bug which is _handleExecuteTransaction() and execute() in BridgeFacet. users won't receive some of their funds.

Tools Used

VIM

Recommended Mitigation Steps

fix the logic and transfer funds after if/else

LayneHaber commented 2 years ago

This will not prevent user from getting the bridged funds, but will only prevent users from getting additional funds.

This feels like more of a value leak than a fund compromise

LayneHaber commented 2 years ago

https://github.com/connext/nxtp/pull/1450/commits/928241db4833c77c661c9b512ad8b3fa2433fd5a

0xleastwood commented 1 year ago

This is actually a false positive, swapExactIn() will actually transfer the asset directly to msg.sender. This was highlighted in the subsequent Spearbit audit as an incorrect fix.

code-423n4 / 2022-06-connext-findings