Veraswap

Cross-chain Swaps

6.1 Failed bridged message cannot be canceled

The Hyperlane allows for replaying failed messages by any relayer by calling process(). However, there is no option to cancel the message. A user who uses Veraswap may perform the bridge and swap action, which bridges the tokens to the destination chain and then bridges the calldata, which is used by the ERC7579 account to perform the swap. If the swap fails (e.g., due to slippage), the user may want to bridge tokens back to the origin chain by submitting another cross-chain call. However, the first swap call, which failed, is still present in the Mailbox and can be executed in the future. If the user were to use Veraswap to perform another swap using the same source token, the first failed swap may be executed by anyone who calls Mailbox.process() with the failed message. As a result, the user's funds would be used for the first swap and not the newest swap.

Recommendation

Since Hyperlane does not allow for cancellation of verified messages that failed on execution, solving the described issue requires handling it on the Veraswap side. One possible scenario would involve stopping the use of the account after bridging back the tokens if the swap fails and the user does not want to execute it. Then on the next call, the new account should be deployed. Another solution could be implementing the registry in the ERC7579ExecutorRouter, which would allow the user to invalidate chosen calldata, so that when it is processed in the Mailbox, the ERC7579ExecutorRouter would not perform any call to the executor.

Resolution

Reduced execution deadline to 10 minutes. Messages cannot be cancelled, but expire by themselves once their deadline is passed. PR

6.2 Incorrect refund address

Sweepers do not specify the sender EOA address as the fee surplus or swap leftover as the recipient. In the StargateBridgeSweep, the _refundAddress is specified as the bridge transfer recipient:

function bridgeAllETH(address payable to, BridgeParams calldata bridgeParams) external {
    ...
    pool.sendToken{value: balance}({_sendParam: sendParam, _fee: fee, _refundAddress: bridgeParams.recipient});

    // Refund user with any excess native tokens
    uint256 leftoverBalance = address(this).balance;
    if (leftoverBalance > 0) {  payable(bridgeParams.recipient).transfer(leftoverBalance);
    }
}

The bridgeParams.recipient can be any address to which the user wants to bridge funds, which does not have to be the user's controlled EOA, but the fee surplus and the ETH leftover will be sent to this address.

In the HypTokenRouterSweep, there are two functions, where only one allows for defining the EOA sender in the hookMetadata as the fee surplus recipient:

// @audit: this function would return the fee surplus to the sweeper
function transferRemote(address router, uint32 destination, bytes32 recipient, uint256 bridgePayment)
// @audit: this function allows for defining the EOA as the refund recipient in the hookMetadata
function transferRemote(
    address router,
    uint32 destination,
    bytes32 recipient,
    uint256 bridgePayment,
    bytes calldata hookMetadata,
    address hook
)

Recommendation

Consider always defining the EOA as the fee surplus and the ETH leftover as a recipient. This can be done by setting the user's EOA as the _refundAddress in the StargateBridgeSweep and defining the EOA address in the hookMetadata with the corresponding hook in the HypTokenRouterSweep. Moreover, ensure that the calls made directly from the EOA account during Bridge & Swap also define the EOA as the refund address.

Resolution

Resolved for Stargate, acknowledged for Hyperlane, although not launch-critical, as refund is limited to excess fees

HypTokenRouterSweep.sol: As mentioned, refundAddress can be encoded in the hookMetadata. The Veraswap frontend does not do this yet, but this is a marginal issue considering the refund amount. Can be fixed client-side. No Solidity changes necessary.

StargateBridgeSweep.sol: Added a refund address parameter bridgeAllETH and bridgeAllToken, commit.

6.3 Lack of target restriction in UniversalRouter

The CALL_TARGET command allows for arbitrary calls to any address, excluding the PERMIT2. This allows malicious users to use the UniversalRouter contract to perform transactions that are against the law. This may include interactions with blacklisted protocols, sending funds to criminals, or using the UniversalRouter as part of the exploit. Since the UniversalRouter is a core part of the Veraswap protocol, such transactions may involve legal or reputation issues. Note that no such scenario has been registered yet; therefore, the probability of such an event is considered minor.

Recommendation

Consider introducing a mapping of allowed target addresses to minimize the legal and reputation risk.

Resolution

This is an accepted risk and is not much different from having the UniversalRouter interact with a malicious Uniswap pool. The entire purpose of the CALL_TARGET command is to enable open modularity, and as such, any allowlist would defeat its purpose. The Veraswap UI only uses the CALL_TARGET command to interact with our audited contracts (eg, HyperlaneSweeper, StargateSweeper).

The changes to the UniversalRouter are released under an open-source license and deployed on-chain as public good infrastructure on which we have no control over post-deployment. Developers are responsible for using the CALL_TARGET command responsibly in their projects.

6.4 transfer(...) used for native currency transfer

The StargateBridgeSweep contract in the bridgeAllETH(...) transfers any leftover ETH to the specified recipient. However, the ETH transfer is performed with the .transfer(...) function. This function limits the passed gas to 2300 weis. While it was designed to protect against reentrancy, it is not a recommended way of transferring ETH, since the receiver address may need more gas to handle such a transfer (e.g., wallets) or the gas cost for specific opcodes may be changed in the future. Please refer to this article for more in-depth information. Note that while .transfer(...) was designed to protect against re-entrancy, those protections should be applied by the CEI pattern or the non-reentrant modifiers.

Recommendation

Consider using .call(...) for native currency transfers.

Resolution

Changed to using CurrencyLibrary in the base Sweep.sol contract that implements shared behaviour between StargateBridgeSweep.sol and HyperlaneSweep.sol. The refund logic is now implemented in the internal _sweep() function of Sweep.sol. Native token refund logic has also been added to the bridgeAllToken function if the user sends too many native tokens to the sweeper.

Commit (diff and reference to the _sweep function)

6.5 Inconsistent application of msg.value in OwnableSignatureExecutor

The executeBatchOnOwnedAccountWithSignature (...) function in the OwnableSignatureExecutor contract executes the batch of transactions on the owned account using msg.value as:

function executeBatchOnOwnedAccountWithSignature(
       SignatureExecutionLib.SignatureExecution calldata signatureExecution,
       bytes calldata signature
   ) external payable {
       // ...
       // execute the batch of transactions on the owned account
       IERC7579Account(signatureExecution.account).executeFromExecutor{value: msg.value}( //@audit signatureExecution value, why msg.value directly. 
           ModeLib.encodeSimpleBatch(),
           signatureExecution.callData
       );
   }

While executeOnOwnedAccountWithSignature (...) function is implemented as:

function executeOnOwnedAccountWithSignature(
     SignatureExecutionLib.SignatureExecution calldata signatureExecution,
     bytes calldata signature
 ) external payable {
     // ....
     IERC7579Account(signatureExecution.account).executeFromExecutor{value: signatureExecution.value}(
         ModeLib.encodeSimpleSingle(),
         signatureExecution.callData
     );
 }

The executeOnOwnedAccountWithSignature (...) function uses the signed msg.value as value: signatureExecution.value while the executeBatchOnOwnedAccountWithSignature (...) function uses the caller's passed msg.value as value: msg.value.

This inconsistency leads to a varied amount of native token, msg.value passed to the user's account. As a result, if the user wanted a specific amount of native token to be added to their account, this is not possible.

Additionally, given that the executeBatchOnOwnedAccountWithSignature (...) function is an external function, anyone can call this function directly with the right signatureExecution and signature data, but with an altered msg.value, which is different from the user-intended signed msg.value. This will successfully execute the target's calls without the intended msg.value, but it will consume the account's nonce, effectively taking away the ability of a user to forward a given native token amount to their account.

Recommendation

Consider using the signatureExecution.value as is done in executeOnOwnedAccountWithSignature(...) function.

Resolution

We have implemented the fix and added comments regarding the purpose of the signatureExecution.value to clarify that it does not impact the value of the calls themselves but rather serves as a constraint on having a deposit to the smart account. The signatureExecution.value will be 0 for most use cases. Commit

6.6 Lack of protection during interaction with Position Managers

The CALL_TARGET command allows for arbitrary calls to any address. This includes managing positions in Uniswap V3 and V4. The commands that manage positions in Universal Router apply restrictions in those scenarios:

function dispatch(bytes1 commandType, bytes calldata inputs) internal returns (bool success, bytes memory output) {
    uint256 command = uint8(commandType & Commands.COMMAND_TYPE_MASK);
    success = true;
    // 0x00 <= command < 0x21
    if (command < Commands.EXECUTE_SUB_PLAN) {
        // 0x00 <= command < 0x10
        ...
        } else {
            // 0x10 <= command < 0x21
            if (command == Commands.V4_SWAP) {
                // pass the calldata provided to V4SwapRouter._executeActions (defined in BaseActionsRouter)
                _executeActions(inputs);
                // This contract MUST be approved to spend the token since its going to be doing the call on the position manager
            } else if (command == Commands.V3_POSITION_MANAGER_PERMIT) {
                _checkV3PermitCall(inputs);
                (success, output) = address(V3_POSITION_MANAGER).call(inputs);
            } else if (command == Commands.V3_POSITION_MANAGER_CALL) {
               //@audit: check the caller and restrict actions
                _checkV3PositionManagerCall(inputs, msgSender());
                (success, output) = address(V3_POSITION_MANAGER).call(inputs);
            } else if (command == Commands.V4_INITIALIZE_POOL) {
                PoolKey calldata poolKey;
                uint160 sqrtPriceX96;
                assembly {
                    poolKey := inputs.offset
                    sqrtPriceX96 := calldataload(add(inputs.offset, 0xa0))
                }
                (success, output) =
                    address(poolManager).call(abi.encodeCall(IPoolManager.initialize, (poolKey, sqrtPriceX96)));
            } else if (command == Commands.V4_POSITION_MANAGER_CALL) {
                // should only call modifyLiquidities() to mint
               //@audit: restrict actions
                _checkV4PositionManagerCall(inputs);
                (success, output) = address(V4_POSITION_MANAGER).call{value: address(this).balance}(inputs);
            } else {
                // placeholder area for commands 0x15-0x20
                revert InvalidCommandType(command);
            }
        }
   ...
}

Those checks protect the users who were tricked into approving their position NFT to the Universal Router, which would allow malicious actors to perform token burns, decrease of liquidity, or collection of rewards. The CALL_TARGET does not perform those checks, leaving this scenario to be potentially exploited.

Recommendation

Consider excluding addresses for which the Universal Router has additional checks when using the CALL_TARGET command.

Resolution

Fully excluded all hard-coded addresses that have dedicated commands for interactions. The attack surface is now significantly reduced. CALL_TARGET can now only be used with addresses that are "out of scope" of the original Universal Router. We've also added a detailed comment explaining where the immutable constants are inherited from.

// Call target cannot be one of the following addresses to reduce the attack surface and enforce proper use of commands
// when interacting with protocol specific addresses
// - address(self): to enforce use of EXECUTE_SUB_PLAN command for self re-entrancy
// - PaymentsImmutables.sol: to avoid arbitrary token transfers
// - UniswapImmutables.sol: to have clear protocol interactions with v2,v3 factories
// - MigratorImmutables.sol: to have clear protocol interactions with v3,v4 position managers
// - ImmutableState.sol (v4-periphery): to have clear protocol interactions with v4 pool actions
if (
    target == address(self) || target == address(WETH9) || target == address(PERMIT2)
        || target == address(UNISWAP_V2_FACTORY) || target == address(UNISWAP_V3_FACTORY)
        || target == address(V3_POSITION_MANAGER) || target == address(V4_POSITION_MANAGER)
        || target == address(poolManager)
) {
    revert CallTargetInvalid(target);
}

6.7 Potential reentrancy during interaction with the bridge

The Veraswap protocol interacts with bridges to transfer tokens and to execute swaps on remote chains. During interaction with the bridge, the fee is sent in native currency. If the fee sent is too large, then the bridge returns the surplus to the specified recipient. Such a recipient can be a contract that can execute any code during a callback with native currency.

Recommendation

Since the bridge interactions are not made mid-execution, we didn't identify any immediate risk. However, this behaviour should be documented and shared with the users. Additionally, the interactions with bridges should be performed as the last action of the Veraswap multicalls.

Resolution

In our current architecture, reentrancy from bridge interactions poses no issue, though this could be a concern in the future. The UniversalRouter is unaffected as it already has reentrancy protection. The sweeper contracts are also unaffected, assuming only one token at a time is sent to the sweeper. This is the case in Veraswap, where users are only making single token pair swaps.

In a more complex scenario, the sweeper contracts could be affected. For example, a user might send the sweeper two tokens to bridge, and then call transferRemote on each of them. In this scenario, a transferRemote call for the first token, followed by a native token refund from the bridging protocol to a malicious recipient, could cause a reentrancy calling transferRemote on the second token and stealing the tokens the user had meant to bridge to themselves.

While not affecting Veraswap's current implementation, we fixed the reentrancy risk to the sweeper contracts by adding a TSTORE-based reentrancy lock similar to UniversalRouter to future-proof the sweeper modules. This was fixed below, where all sweepers now inherit from Sweep.sol, which inherits from Lock.sol that implements a TSTORE-based lock. All relevant functions are then protected using the isNotLocked modifier.

6.8 Transfers of funds are not checked to be successful

The UniversalRoutercontract's dispatcher logic handles a CALL_TARGET command, which is designed to execute arbitrary calls to external contracts. This is achieved using a low-level call(...). Low-level calls like .call do not propagate reverts from the callee. Instead, they return a boolean value indicating whether the call succeeded or failed.

// ...
function dispatch(...) internal {
    // ...
    } else if (command == Commands.CALL_TARGET) {
        (address target, uint256 value, bytes calldata data) = CalldataCallTargetDecoder.decodeCallTarget(
                    inputs
                );
          if (target == address(PERMIT2)) revert CallTargetPermit2();
         
        // @audit The `success` boolean is not checked.
        (success, output) = payable(target).call{value: value}(data);
        // ...
    }
    // ...
}

The UniversalRouter command structure includes an allowRevert flag to control whether a command's failure reverts the entire transaction. However, it allows for failure during the tokens or native currency transfers. This differs from other commands, which utilize the Payments contract, which performs funds transfers with safeTransferETH(...) and safeTransfer(...) that require always successful transfers.

Recommendation

Consider properly documenting described behaviour.

Resolution

Added comments detailing the CALL_TARGET command's behaviour, its similarity with other commands that return success, and its difference from commands that always revert, such as those that handle token transfers.

// Call may fail without a revert if Commands.FLAG_ALLOW_REVERT is set
// This behavior is similar to other commands that use .call (eg. V3_POSITION_MANAGER_CALL, V4_POSITION_MANAGER_CALL)
// This behavior is different other commands that transfer ETH or tokens (eg. TRANSFER)
(success, output) = payable(target).call{value: value}(data);