DeFi Saver v3.1 Add Safe Wallet Support (delta)

The protocol is deployed on several L2 blockchains, namely Arbitrum, Base and Optimism, which use a centralized sequencer that executes and rolls up the L2 transactions by batching multiple transactions into a single transaction. Thus, if the sequencer is down, messages cannot be transmitted from L1 to L2 and no L2 transactions are executed. In that case chainlink price oracles might have stale prices leading to strategies triggering untimely or being executed at the wrong prices. Chainlink recommends using their L2 sequencer uptime feeds to be able to diagnose downtimes and act accordingly, e.g., revert if the sequencer is down (message/tx sent from L1 to L2 and queued until the sequencer is back up) or even if it is up for a small period of time.

In the TokenPriceHelper contract, there are several functions that fetch token prices from different price feeds including Chainlink and Aave. However, the prices returned are not validated nor checked for staleness. Even though the impact of using stale values may be restricted, it is recommended to add such checks to ensure that all the consumed prices are fresh.

As far as the Chainlink oracles are concerned, the following checks could be used to detect stale or invalid values:

require(updatedAt != 0);
require(updatedAt <= block.timestamp);
require(block.timestamp - updatedAt <= STALE_PRICE_DELAY);

Regarding the STALE_PRICE_DELAY initialization, the oracles perform updates when their deviation threshold is crossed or when their heartbeat time expires. As a result, the STALE_PRICE_DELAY could be set to a value relative to the predefined heartbeat to ensure that if no updates have occurred until then, the returned value is considered stale. Since different feeds have different heartbeats, a mapping from feed to heartbeat should be used instead of a single general and thus inaccurate heartbeat for all the feeds.

The function TokenUtils::approveToken is responsible for handling token approvals.

function approveToken(
    address _tokenAddr,
    address _to,
    uint256 _amount
) internal {
    if (_tokenAddr == ETH_ADDR) return;

    if (IERC20(_tokenAddr).allowance(address(this), _to) < _amount) {
        // Dedaub: safeIncreaseAllowance could be used here
        IERC20(_tokenAddr).safeApprove(_to, _amount);
    }

    // Dedaub: if the new allowance (_amount) is less than the old,
               safeDecreaseAllowance could be used
}

The aforementioned function allows only approval increases. It is understood that it is expected to be used in such a context and that there are other functions that support approval decreases. Nevertheless, we would suggest also supporting approval decreases via this function for completeness reasons. Regarding approval increases, it is recommended to perform them using a safeIncreaseAllowance implementation instead of a plain safeApprove, even though it is understood that approvals will be issued in a context where the spender is not expected to front-run the user.

In the StrategyExecutor contract, the executeStrategy function is used by the authorized bots to execute single and bundled strategies. For the bundled strategies case, there exists a scenario where some of the bundled strategies might get blocked/fail to execute.

Strategies can be defined as continuous, which would allow an action to be repeated multiple times. The executeRecipeFromStrategy function of the RecipeExecutor contract, however, deactivates the subscription ID (_subID) related to a bundle of strategies if one of them is executed and is not continuous.

function executeRecipeFromStrategy(
    uint256 _subId,
    ...
) public payable {
    ...
    // Dedaub: When the executed strategy is not continuous
    //         the _subId is being deactivated
    if (!strategy.continuous) {
        SubStorage(SUB_STORAGE_ADDR).deactivateSub(_subId);
    }
    ...
}

As a result, a bot that is meant to execute the strategies of a bundle could be blocked in case one of them has been declared as non-continuous. Consider the following steps for example:

• StrategyStorage::createStrategy is used to create the desired strategies

• One of them is initialized as non-continuous

• BundleStorage::createBundle is used to create a new bundle out of these strategies

• A bot calls the StrategyExecutor::executeStrategy providing the _subId of the bundle and the index of the strategy which should be executed

• Assume that the index points to the strategy which was created as non-continuous

• The call delegates to the RecipeExecutor::executeRecipeFromStrategy function which executes the strategy

• During this call, however, the check:

  if (!strategy.continuous) {
      SubStorage(SUB_STORAGE_ADDR).deactivateSub(_subId);
  }
  

will deactivate the used _subId, which would be the bundle ID

• Then, when the bot will try to execute another strategy from the bundle this would fail due to the following check it will fail since the bundle would have been deactivated:

StoredSubData memory storedSubData =
  SubStorage(SUB_STORAGE_ADDR).getSub(_subId);

if (!storedSubData.isEnabled) {
    revert SubNotEnabled(_subId);
}

Even though this does not seem to pose any direct threat or DoS in strategy execution, it would make sense to ensure that allowing bundling continuous with non-continuous strategies is the desired behavior and would not introduce any issues in future changes.

A possible use case in allowing non-continuous strategies to be bundled with continuous ones would be to add the non-continuous strategy as the last strategy of the bundle to deactivate the _subId after executing all of them to prevent re-execution by the bot.

The variable userProxy of the StrategyModel::StoredSubData struct should be renamed, possibly to wallet. Obsolete references to the ds-proxy can also be found in the SubStorage contract, namely in the Subscribe event and in the @dev comment of the SubStorage::updateSubData function.

The variable user of the DFSExchangeData::ExchangeData struct is never used.

In CurveUsdSwapper::takeSwapFee, the amount _sellAmount / _dfsFeeDivider can be assigned to feeAmount instead of being added to it, as feeAmount is 0.

In some of the contracts that contain the hardcoded addresses there are some TODOs or NOT LIVE ADDRESS left in comments. Moreover, some of the addresses used seem to be placeholders. For example, in ArbitrumCoreAddress the 0xEeee...EEEeE is used for the MODULE_AUTH_ADDR. We raise this here for visibility and to ensure that all the addresses point or have been updated to the correct ones for all the modules.

In several of the contracts that contain the hardcoded addresses, some of the addresses are declared as constant internal when the majority of them are declared as address internal constant. We are mentioning this with uniformity in mind, since we found similar changes among the commits under scope.

The code is compiled with Solidity 0.8.10. Version 0.8.10, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.