Swapper Manual Review Findings

SRE-01M: Potentially Dangerous Low-Level Call

Type	Severity	Location
Language Specific		Swapper.sol:L24

Description:

The Swapper::swap function will perform a low-level call to the _swapRouter without any validation of its existence. This will result in "successful" calls to any contract that does not contain code (i.e. the address(0) or the caller's address).

Impact:

The Swapper::swap function will "successfully" execute even if performs no actual call due to invoking a _swapRouter with no code in it.

Example:

Copy
16function swap(
17    IERC20 _srcToken,
18    IERC20 _dstToken,
19    uint256 _amountIn,
20    address _swapRouter,
21    bytes calldata _data
22) external {
23    _srcToken.approve(_swapRouter, _amountIn);
24    (bool success, bytes memory returndata) = _swapRouter.call(_data);
25    uint256 length = returndata.length;
26    if (!success) {
27        // call failed, propagate revert data
28        assembly ("memory-safe") {
29            revert(add(returndata, 32), length)
30        }
31    }
32    _srcToken.approve(_swapRouter, 0);
33
34    // send tokens back to caller
35    uint256 balance = _srcToken.balanceOf(address(this));
36    if (balance > 0) {
37        _srcToken.safeTransfer(msg.sender, balance);
38    }
39
40    balance = _dstToken.balanceOf(address(this));
41    if (balance > 0) {
42        _dstToken.safeTransfer(msg.sender, balance);
43    }
44}

Recommendation:

We advise the Swapper::swap function to at the very least ensure that the _swapRouter has been defined and as a recommended course of action to ensure that the _swapRouter is one of the pre-authorized targets of the Swapper.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The _swapRouter is now validated as being non-zero, preventing misconfigured calls to the zero-address from successfully executing. To note, a better alleviation would be to ensure that code is located within the _swapRouter but we consider the present validation as the minimum required for this exhibit to be marked as addressed.

SRE-02M: Insecure Arbitrary Interactions

Type	Severity	Location
Language Specific		Swapper.sol:L24

Description:

The Swapper contract is meant to be utilized as an ephemeral contract that will conduct DEX trades on behalf of another party, however, it does not contain any access control and performs an arbitrary call.

This permits exploitation of lingering state changes, such as approvals that are never cleared.

Impact:

A medium severity has been assigned to this vulnerability due to the triviality of exploiting its flaw (arbitrary uncontrolled calls) and its impact, which could result in extended denial-of-service attacks (f.e. by hijacking USDT trades with a non-zero approval).

Example:

Copy
9/// @notice Swapper is a helper contract for sending calls to arbitray swap router
10/// @notice Since there's no need to approve tokens to Swapper, it's safe for Swapper
11/// @notice to call arbitrary contracts.
12contract Swapper {
13
14    using SafeERC20 for IERC20;
15
16    function swap(
17        IERC20 _srcToken,
18        IERC20 _dstToken,
19        uint256 _amountIn,
20        address _swapRouter,
21        bytes calldata _data
22    ) external {
23        _srcToken.approve(_swapRouter, _amountIn);
24        (bool success, bytes memory returndata) = _swapRouter.call(_data);
25        uint256 length = returndata.length;
26        if (!success) {
27            // call failed, propagate revert data
28            assembly ("memory-safe") {
29                revert(add(returndata, 32), length)
30            }
31        }
32        _srcToken.approve(_swapRouter, 0);
33
34        // send tokens back to caller
35        uint256 balance = _srcToken.balanceOf(address(this));
36        if (balance > 0) {
37            _srcToken.safeTransfer(msg.sender, balance);
38        }
39
40        balance = _dstToken.balanceOf(address(this));
41        if (balance > 0) {
42            _dstToken.safeTransfer(msg.sender, balance);
43        }
44    }
45}

Recommendation:

We advise the Swapper::swap to either impose access control, restrict the targets it can invoke, restrict the function signatures that it can invoke, or any combination of these to ensure an adequate operational security level for the contract.

Alleviation (302b96f324a88038a0872015466cd43783c14543):

The Teahouse Finance team evaluated the desired operational security of the Swapper contract and has opted to enforce access control in the Swapper::swap function ensuring a list of allowed callers is able to invoke it.