Tokemak V2 Update Security Review Report

There are no restrictions on calling the initialize function, so can call it at any time to modify important variables. This allows for limited changes to the poolAddress, which may result in price updates not functioning correctly.

/// @inheritdoc IStatsCalculator
/// @dev On this calculator, if submitting a boosted token aprId will be for ERC4626.asset() token
function initialize(bytes32[] memory dependentAprIds, bytes memory initData)
    external virtual override {
    [..]
}

Like other functions, it should have the initializer modifier to prevent the subsequent calling of the initialize function.

The NavTracking library implements a State struct to hold at most 91 values of the NAV history. It also keeps an index, length and timestamp.

In the insert function, there is the logic that if the current timestamp is less than 1 day after then last timestamp, then it won't move the index further and overwrite the same value each time. However, the length (len) is still increased in this case. As a result, the len could be as high as 91, while the history array only contains an element at index 0.

This could lead to DoS reverts when the function state.getDaysAgo is used, as it would revert with an array out-of-bounds error.

The function is used inside of AutopoolETHStrategy:navUpdate, which is used by the Autopool itself. These function check the state.len to make sure the day entry exists, but in this case it would still lead to a revert and potential DoS.

function insert(State storage self, uint256 navPerShare, uint40 timestamp) internal {
    if (timestamp < self.lastFinalizedTimestamp) revert InvalidNavTimestamp(self.lastFinalizedTimestamp, timestamp);
 
    // if it's been a day since the last finalized value, then finalize the current value
    // otherwise continue to overwrite the currentIndex
    if (timestamp - self.lastFinalizedTimestamp >= 1 days) {
        if (self.lastFinalizedTimestamp > 0) {
            self.currentIndex = (self.currentIndex + 1) % MAX_NAV_TRACKING;
        }
        self.lastFinalizedTimestamp = timestamp;
    }
 
    self.history[self.currentIndex] = navPerShare;
    if (self.len < MAX_NAV_TRACKING) {
        self.len += 1;
    }
 
    emit NavHistoryInsert(navPerShare, timestamp);
}

The self.len increment on lines 33-35 should be moved inside of the earlier conditional when the index is incremented.

The SlippageBudgetHook is a hook that can set a slippage budget for an Autopool on a rebalance. For each rebalance, it can use some amount of the budget during a time period, to protect against heavy slippage.

It calculates the slippage by taking the parameter values (assetOut, amountOut) and (assetIn, amountIn) and calculates the valueOut and valueIn using the _getBaseValue function. This function using the spot price of the DestinationVault directly.

Afterwards, the difference of valueOut and valueIn is compared to the AUM, which is retrieved using IAutopool(msg.sender).totalAssets(IAutopool.TotalAssetPurpose.Global). The function totalAsset will return the normal price for Global, which is calculated from the average between the safe and spot price as can be seen in AutopoolDebt.sol:_recalculateDestInfo:

(uint256 spotPrice, uint256 safePrice, bool isSpotSafe) = destVault.getRangePricesLP();
uint256 minPrice = spotPrice > safePrice ? safePrice : spotPrice;
uint256 maxPrice = spotPrice > safePrice ? spotPrice : safePrice;
[..]
uint256 newDebtValue = (minPrice * currentShares + maxPrice * currentShares) / (div * 2);

This means that the value and the AUM are using different prices of the assets by default, depending on how much the safe and spot price diverged.

The difference will lead to some amount of default slippage every time, even though this slippage never happened. Depending on the amount, it can eat away at the slippage budget.

function onRebalanceStart(ProcessRebalanceParams calldata args, address)
external override {
    [..]
    // Calculate slippage in basis points
    (uint256 valueOut,) = _getBaseValue(params.destinationOut, params.tokenOut, params.amountOut);
    (uint256 valueIn,) = _getBaseValue(params.destinationIn, params.tokenIn, params.amountIn);
    uint256 aum = IAutopool(msg.sender).totalAssets(IAutopool.TotalAssetPurpose.Global);
 
    // Slippage = (valueOut - valueIn) * 10000 / totalAssets
    uint256 slippageBps = ((valueOut - valueIn) * BPS_DENOMINATOR) / aum;
    [..]
}

The function _getBaseValue should use the average between the safe and spot price to get the same asset price that was used for the totalAssets(Global) AUM calculation.

The LstPriceHook allows for setting a tolerance factor per Autopool to check the difference of the safe and spot price of an LST against a tolerance.

The function _configureAutopool sets this tolerance but does not validate the input value. Since the tolerance is a factor in 1e18 decimals and used compare the ratio between safe/spot, it should be capped to a sensible value, such as 1e18 (100%).

function _configureAutopool(address autopool, uint256 tolerance) private {
    Errors.verifyNotZero(tolerance, "tolerance");
    autopoolTolerance[autopool] = tolerance;
    emit AutopoolToleranceConfigured(autopool, tolerance);
}

In various function there are literals used instead of named constants:

• onRebalanceStart on line 108: 1e18 and 10_000
• _configurateAutopool on line 154 and 157: 1 minutes and 30 days

function _configureAutopool(address autopool, uint40 minSecondsGaps, uint40 idleMinSecondsGaps) private {
    if (minSecondsGaps < 1 minutes || minSecondsGaps > 30 days) {
        revert Errors.InvalidParam("minSecondsGap");
    }
    if (idleMinSecondsGaps < 1 minutes || idleMinSecondsGaps > 30 days) {
        revert Errors.InvalidParam("idleMinSecondsGap");
    }
    [..]
}

We recommend to use named constants instead of literals to improve readability and code quality as refactoring a parameter value would be easier if it exists in multiple places.

In the _onUnregistered function, the entry of the given Autopool is erased by writing a new struct with only zero values. We recommend to simplify this by using delete on the entry directly.

function _onUnregistered(
    bytes memory
) internal override {
    autopoolData[msg.sender] = AutopoolData({
        minSecondsGap: 0,
        idleMinSecondsGap: 0,
        lastRebalanceTimestamp: 0,
        idleLastRebalanceTimestamp: 0
    });
    emit AutopoolMinTimeGapConfigured(msg.sender, 0, 0);
}

The function withdrawBaseAsset does not perform any access control checks on the caller.

The internal function _withdrawBaseAsset ensures this by burning shares from the caller, but since the Autopool is the only one that can mint these shares, it could be safer to also put the same access control on withdrawBaseAsset in case the Autopool has an arbitrary transfer vulnerability, making the DestinationVault shares useless if they were to end up in an attacker's possession.

/// @inheritdoc IDestinationVault
function withdrawBaseAsset(
    uint256 shares,
    address to
) external returns (uint256 amount, address[] memory tokens, uint256[] memory tokenAmounts) {
    return _withdrawBaseAsset(msg.sender, shares, to);
}