Vault functions

Note: If we make the assumption that the orchestrators are safe, the only function that represents a substantial risk is removeLiquiditySwap, being called by the executor contract, and therefore dependant on the underlying executor’s contract security. revertOrder also represents a risk, but the impact would be up to the total amounts of order that are unfulfilled

Problem with the executor

Exploit through quoting engine

The following flow illustrates the problem with using the executor as a “middleman” between the orchestrator the vault contract. If there is any vulnerability in the quoting engine, all the vaults can be drained to up to 100% of their funds by exploiting the executor.

This is caused by the combination of the executor contract having the power to withdraw any amount from the vaults, and his ability to perform arbitrary calls ordered by the orchestrator (→ quoting engine behind the scene)

This risk can be easily mitigated by removing any permissions given to the executor contract, while giving it to the orchestrators. The arbitrary call used for multicall can be passed directly from the vault to a secure isolated execution environment (the executor becomes all of that), to limit the risk to only the current order amount → the vault would be passed an arbitrary calldata, and would first, transfer the order amount to the secure execution contract (executor), and then send the calldata. The executor will have no permissions whatsover nor any state, limiting the risks of its usage. The following schema demonstrate the proposal.

Note: the same system can be applied to the revertOrder

The impact surface of a malicious quote is reduced from the whole balances of vaults, to only the moving assets.

Changes proposal

1. The following changes would allow the executor contract not to be a sensitive part of the protocol anymore, and reduces the attack vectors to only the moving assets (in opposition to all the assets held in the vault without those changes). With those changes, the executor could be removed from the scope of the audit, reducing the attack surface and audit to only the vault.
2. Also, making the executor “powerless” means that there is no risk anymore in arbitrary quotes being sent from the client (user directly), therefore removing the necessity to upload quotes on IPFS and verify them within the lit actions, as both the quoteIn an quoteOut will be executed in secure environments (quoteIn and quoteOut on the executor that doesn t have any permissions)

On the Vault:

1. Change the *removeLiquiditySwap* to add arbitrary call data and change the *onlyExecutor* to *onlyOrchestrator*

   function removeLiquiditySwap(
           Order memory order,
           targets memory address[],
           data memory bytes[],
           values memory uint256[]
       ) external virtual override onlyExecutor whenNotPaused {
       ...
       
       transferErc20(STABLECOIN, address(address(EXECUTOR)), order.amountIn);
       EXECUTOR.aggregate(targets, data, values);
   

2. Change the revertOrder function in a similar manner