connext / connext/chain-abstraction-integration

Enforce the swapper allowlist on source-chain direct swap calls

Open
#53 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Solidity
Stars
16
Forks
12
PR merge metrics
No merged PRs in 30d

Description

Hi Connext team, I found a route-validation gap in the chain-abstraction integration contracts: `SwapAdapter.exactSwap()` enforces `allowedSwappers`, but the source-chain `SwapAndXCall` path bypasses it by calling the public `directSwapperCall()` helper, which does not check the allowlist.

## Impact

The owner-managed `allowedSwappers` mapping suggests that swap execution should be restricted to known adapters/routers. On the source-chain `swapAndXCall()` path, however, a caller supplies `_swapper` and `_swapData`; `_setupAndSwap()` approves `_swapper` for the input token and then calls `this.directSwapperCall(_swapper, _swapData)`. `directSwapperCall()` performs a low-level call to that arbitrary address without checking `allowedSwappers`.

At minimum, this means the actual source-chain route can differ from the locally intended allowlisted route set. It also leaves the contract making arbitrary external calls as itself after granting `_swapper` a max approval for `_fromAsset`. Even if the common case only risks the caller's own transferred input, this is a sharp edge for residual/stuck balances and for systems relying on the allowlist as a route-safety invariant.

## Code

`contracts/shared/Swap/SwapAdapter.sol:23-30` defines the allowlist:

```solidity
mapping(address => bool) public allowedSwappers;

address public immutable uniswapSwapRouter = address(0xE592427A0AEce92De3Edee1F18E0157C05861564);

constructor() {
allowedSwappers[address(this)] = true;
allowedSwappers[uniswapSwapRouter] = true;
}
```

`contracts/shared/Swap/SwapAdapter.sol:63-84` enforces it for `exactSwap()`:

```solidity
function exactSwap(
address _swapper,
uint256 _amountIn,
address _fromAsset,
address _toAsset,
bytes calldata _swapData // comes directly from API with swap data encoded
) external payable returns (uint256 amountOut) {
require(allowedSwappers[_swapper], "!allowedSwapper");

// If from == to, no need to swap
if (_fromAsset == _toAsset) {
return _amountIn;
}

if (_fromAsset == address(0)) {
amountOut = ISwapper(_swapper).swapETH(_amountIn, _toAsset, _swapData);
} else {
if (IERC20(_fromAsset).allowance(address(this), _swapper) < _amountIn) {
TransferHelper.safeApprove(_fromAsset, _swapper, type(uint256).max);
}
amountOut = ISwapper(_swapper).swap(_amountIn, _fromAsset, _toAsset, _swapData);
}
}
```

But `contracts/shared/Swap/SwapAdapter.sol:87-96` exposes a direct call helper with no allowlist check:

```solidity
/**
* @notice Swap an exact amount of tokens for another token. Uses a direct call to the swapper to allow
* easy swaps on the source side where the amount does not need to be changed.
* @param _swapper Address of the swapper to use.
* @param swapData Data to pass to the swapper. This data is encoded for a particular swap router.
*/
function directSwapperCall(address _swapper, bytes calldata swapData) external payable returns (uint256 amountOut) {
bytes memory ret = _swapper.functionCallWithValue(swapData, msg.value, "!directSwapperCallFailed");
amountOut = abi.decode(ret, (uint256));
}
```

`contracts/origin/Swap/SwapAndXCall.sol:109-127` approves the caller-supplied `_swapper` and reaches that unchecked helper:

```solidity
function _setupAndSwap(
address _fromAsset,
address _toAsset,
uint256 _amountIn,
address _swapper,
bytes calldata _swapData
) internal returns (uint256 amountOut) {
TransferHelper.safeTransferFrom(_fromAsset, msg.sender, address(this), _amountIn);

if (_fromAsset != _toAsset) {
require(_swapper != address(0), "SwapAndXCall: zero swapper!");

// If fromAsset is not native and allowance is less than amountIn
if (IERC20(_fromAsset).allowance(address(this), _swapper) < _amountIn) {
TransferHelper.safeApprove(_fromAsset, _swapper, type(uint256).max);
}

amountOut = this.directSwapperCall(_swapper, _swapData);
} else {
amountOut = _amountIn;
}
```

The ETH path does the same unchecked direct call. `contracts/origin/Swap/SwapAndXCall.sol:147-158`:

```solidity
function _setupAndSwapETH(
address _toAsset,
uint256 _amountIn,
address _swapper,
bytes calldata _swapData
) internal returns (uint256 amountOut) {
require(msg.value >= _amountIn, "SwapAndXCall: msg.value != _amountIn");

if (_toAsset != address(0)) {
require(_swapper != address(0), "SwapAndXCall: zero swapper!");
amountOut = this.directSwapperCall{value: _amountIn}(_swapper, _swapData);
```

The destination swap-forwarding path uses `exactSwap()` and therefore does enforce the allowlist. `contracts/destination/xreceivers/Swap/SwapForwarderXReceiver.sol:31-43`:

```solidity
function _prepare(
bytes32 _transferId,
bytes memory _data,
uint256 _amount,
address _asset
) internal override returns (bytes memory) {
(address _swapper, address _toAsset, bytes memory _swapData, bytes memory _forwardCallData) = abi.decode(
_data,
(address, address, bytes, bytes)
);

uint256 _amountOut = this.exactSwap(_swapper, _amount, _asset, _toAsset, _swapData);
```

## Suggested fix

Apply the same allowlist invariant to source-chain direct calls:

- Add `require(allowedSwappers[_swapper], "!allowedSwapper");` to `directSwapperCall()`, or make it internal and call it only after validation.
- Alternatively, have `_setupAndSwap()` and `_setupAndSwapETH()` use `exactSwap()` or explicitly check the allowlist before approving/calling `_swapper`.
- Consider resetting temporary approvals after the swap instead of leaving max approvals to user-supplied swapper addresses.
- Add tests proving `swapAndXCall()` rejects a non-allowlisted `_swapper` for both ERC20 and ETH input routes.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in contracts/shared/Swap/SwapAdapter.sol by comparing exactSwap() with directSwapperCall(), then trace the ERC20 and ETH paths in contracts/origin/Swap/SwapAndXCall.sol. Add or run tests for swapAndXCall() with non-allowlisted swappers on both input routes; done means those calls are rejected while allowlisted routes retain their expected behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
solidity
Domain
blockchain, security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.