connext / connext/chain-abstraction-integration
Enforce the swapper allowlist on source-chain direct swap calls
- 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