Belt Crosschain AMM Audit

The request.destChainSwapData.exchange, used to perform a “local” swap at the end of a cross-chain swap, is controlled by the user and could be set to an arbitrary malicious contract. Such a malicious exchange could be used to steal any token potentially owned by the Router (not just tokens transferred to the Router by the user during a swap).

Such an attack is also facilitated by the following issues in the Router contract.

• In _swap, an approval is given to the exchange in order to do the swap, but it is not removed afterwards, and could potentially remain valid at the end of the call.
• In _swap, the code succeed = amountOut >= request.amountOut ignores the original succeed status, the Router will think that a swap failed even if the address(proxy).call transaction did not revert.
• onTokenBridgeReceived has no reentrancy guard, allowing a reentrancy of the form onTokenBridgeReceived-> _swap -> proxy.call -> swap -> _swap

The above issues allow the following three attacks (which are related but substantially different in their logic), used to steal any token owned by the Router.

• request.destChainSwapData.exchange is set to a malicious contract which simply steals the tokens to be swapped, and sends nothing back to the Router (without reverting). The Router, due to the succeed = amountOut >= request.amountOut code, believes that the swap failed, so it returns the tokens to the user, so the user receives the tokens twice.
• request.destChainSwapData.exchange is set to a malicious contract which simply reverts. The Router, since the swap failed, returns the token to the user, but the user can collect the tokens a second time via the approval that the malicious contract still retains for the tokens.
• Using Router.swap an adversary can perform a “pseudoTransfer”, meaning a transfer of tokens to the router while retaining an approval on the transferred tokens (this is possible using again a malicious exchange, and the fact that the approval remains valid after the swap, if the exchange does not consume it). A pseudoTransfer increases the Router’s balance artificially, making it appear that the Router has more tokens, while the adversary can reclaim them at any time via his approval.

Such pseudoTransfer can be used in an attack as follows: destChainSwapData.exchange is set to a malicious contract which first performs a normal swap. Then, it reenters in Router.swap, and performs a “pseudoTransfer”, virtually increasing the Router’s balance for the swap’s target token. After the exchange finishes, amountOut is arbitrarily large (because of the pseudoTransfer), which leads to the Router returning more tokens than it should.

The severity of the above attacks is reduced by the fact that the Router does not currently seem to hold any tokens by design (so there’s nothing to steal, except for tokens that the Router received as extraneous fee or by accident). Still, the Router could potentially hold tokens in some future update, and the attack is very broad, allowing to steal any amount from any token owned by the Router.

To fix this vulnerability and strengthen security checks, we suggest:

• Limiting request.destChainSwapData.exchange to a whitelist of pre-approved exchange proxies.
• To zero the approval to the exchange proxy after the swap.
• To have succeed = false only if the exchange reverted, in which case we are sure that the tokens to be swapped are left intact (no need to check the swapped amount, both the exchange proxy and the exchange itself should already perform such a check).
• Add nonReentrant to Router.onTokenBridgeReceived.

In the current architecture, there are two types of Orbit fees:

• User-side: Orbit fee (or tax) to transfer the assets from the source chain to the Orbit chain where the swap takes place. These are paid by the user.

• Belt-side: Orbit fee to transfer the swapped assets to the target chain, or send the assets back to the source chain (_giveBack), in case of swap failure. These are paid by Belt without explicitly collecting any funds from the user to cover them.

As a consequence, a malicious user can force Belt to repeatedly pay the Belt-side fees by sending “dummy” swap requests. These can either be swaps that fail (with small amountOut), causing a _giveBack, or even swaps that succeed for small amounts. Depending on the amount of the User-side fees, a malicious user might have incentives to perform such an attack. Note that there are source chains without any fee (e.g., BSC), which facilitates such an attack. Note also that an Orbit tax alone (a percentage of the transferred amount) cannot prevent this attack, since the user is free to ask for swaps of arbitrarily small amounts.

One possible solution would be to collect a fee from the user to cover for the Belt-side Orbit fees.

In SwapReceiver::onTokenBridgeReceived, if _requestSwap fails, the tokens are sent back to the user (via _giveBack). However, if _requestSwap succeeds but _requestBridge fails, the tokens are not sent back (since they are already swapped). The comment in line 197 suggests that an escrow account will be used to store the funds in this case.

An idea to consider is: to avoid an escrow account, both the _requestSwap and _requestBridge operations can be performed inside an internal transaction. If _requestBridge fails, the transaction can be reverted, which will also undo the swap, restoring the source tokens. Then _giveBack can be simply used to send the source tokens back to the user (similarly to the case when the swap fails).

Note that this is just a functional suggestion, not a security issue.

The recipient argument of Router::_requestBridge should best be renamed to receiver, to match the distinction between the two concepts throughout the rest of the code.

In Router, there is some unnecessary ABI encoding/decoding of TxRequests:

function swapAndCrossChain(... TxRequest memory txRequest) ... {
   ...
   _crossChain(swapRequest.path[swapRequest.path.length - 1], amountOut,
               abi.encode(txRequest));
}

function crossChain(... TxRequest memory txRequest) ... {
   ...
   _crossChain(token, amount, abi.encode(txRequest));
}

function _crossChain(address token, uint amount, bytes memory txData) private {
   ...
   TxRequest memory txRequest = abi.decode(txData, (TxRequest));
   _requestBridge(token, txRequest.receiver, amount, "ORBIT",
                  abi.encode(txRequest));  // Dedaub:same as txData
   ...
}

Either the last encoding can be removed, or the earlier two, so that _crossChain takes a TxRequest and encodes it.

The code of SwapReceiver::onTokenBridgeReceived has the form:

function onTokenBridgeReceived(address _token, ...) ... {
   ...
   bool isValidSender = false;
   for (uint i = 0; i < minters.length; i ++) {
      isValidSender = isValidSender || minters[i] == msg.sender;
   }
   ...

It is not clear why any minter would be acceptable as the msg.sender. Isn’t minterByToken[_token] the only valid one? If not, this should be documented. Even though minters are trusted and registered, “confused deputy” attacks through trusted but misused entities are a common threat pattern. Furthermore, if there really needs to be a search for the right minter, the loop could be optimized to:

function onTokenBridgeReceived(address _token, ...) ... {
   ...
   bool isValidSender = false;
   for (uint i = 0; i < minters.length; i ++) {
      if (minters[i] == msg.sender) {
         isValidSender = true;
         break;
      }
   }
   ...

Although this is just an advisory item (since we cannot currently see security implications), unless there is a strong reason for iterating over possible minters, we strongly recommend addressing it. It is exactly the kind of “loose privilege” that an attacker may try to exploit.

The overall readability of the codebase could significantly benefit from comments, e.g. above crucial functions, that describe the intended functionality in respect to the overall service’s workflow and/or invariants that are to be respected under normal circumstances.

Most of the contracts were compiled with the Solidity compiler v.0.6.12 while some of them with v.0.5.0. Both versions have some known bugs at the time of writing. We have inspected the list of bugs and believe that they do not affect the audited contracts.