Stable Jack

The protocol is complex and its correctness depends on consistent external configuration and use. The audit assumes that all initialization is performed correctly, with consistent parameters that permit further operation and upgrade of the contracts, when necessary. Furthermore, the audit assumes that the protocol admin provides the right parameters to calls, e.g., appropriate minAmountOut values to functions that take such parameters because they perform on-chain token swaps. Similarly, the base commit of the code assumes in several places that the tokens used support 18-decimal precision, which is an assumption that we also make and avoid discussing potential issues with decimals.

These are "obvious” considerations, but given the complexity of the protocol, there is significant potential for error.

Additionally, the code for checking (Chainlink) oracle prices is commented-out in the final audited version, for testing under controlled conditions. We optimistically assume that the code will be correctly re-inserted for production deployment. (In the base commit, the oracle code is partial—e.g., assumes it is only to get the price of LST tokens.)

It is not entirely clear how well the protocol will operate if under-collateralized (i.e., the total value of base token currently held is below the value of aTokens minted), in terms of the code. For instance, some parts of the code assume the aToken value to be $1, whereas others permit a partial value in the case of under-collateralization. However, the point is relatively moot because the protocol itself should always avoid getting in under-collateralized mode, by appropriate and timely calls to Treasury::rebalanceUp. (Such calls can only be performed by the protocol admin.) If the protocol enters under-collateralized mode, there is no possible recovery by attracting investors (nor a possibility to invest while in this mode). The only potential recovery is by a rise in the price of the underlying (yield-bearing) token.

Therefore, under-collateralization is not desired anyway, and we do not belabor the case of under-collateralization, since it should be avoided via timely admin action.

The protocol receives a price estimate for a token swap by querying the DEX that will be used for the swap itself. This is unrelieable and prone to attack, since the DEX pool can be tilted to return “unfair” prices before a forced swap, or before an admin-initiated swap that gets front-run. The issue applies to two spots in the code, ProtocolUtils::swapTokens and Treasury::_swapTokens, shown below:

uint256 deadline = block.timestamp + 30;
...
uint256[] memory amountsOut = router.getAmountsOut(amount, path);
uint256 expectedOutAmount = amountsOut[amountsOut.length - 1];

// Calculate minimum acceptable amount with 0.05% slippage tolerance
uint256 minOutAmount = FullMath.mulDiv(expectedOutAmount, (10_000 - 5), 10_000);
...
try
router.swapExactTokensForTokens(amount,minOutAmount,path,address(this),deadline)
returns (uint256[] memory amounts) {
  swappedAmount = amounts[amounts.length - 1];
  if (swappedAmount < minOutAmount)
     ITreasury.ErrorSwapFailed.selector.revertWith();
}

We additionally note in the above code two advisory issues: the use of numeric constants in the code instead of giving them a symbolic name and the use of a deadline (current block + 30) which has no meaning in on-chain calls to a DEX router: the swap will either be performed synchronously in the current block or not at all.

RebalancePool::harvestFees() calls the following function to convert a DXToken amount to a base token amount. However, the function returns a dollar amount, not a base token amount.

function dxTokenToBaseToken(uint256 dxTokenAmount) external view override returns (uint256 baseTokenAmount) {
	uint256 navPerToken = nav();
	baseTokenAmount = (dxTokenAmount * navPerToken) / PRECISION;
       // Dedaub: nav() seems to be in dollars, not in base token
}

There seem to be no tests covering the harvestFees functionality.

When minting new aTokens, the Protocol contract deposits them in the RebalancePool, and removes them from the pool during redeem. The RebalancePool::redeem function imposes a COOLING_PERIOD after the last deposit, during which redeems are not possible.

function redeem(uint256 shares, address receiver, address owner) public override(ERC4626, IERC4626) nonReentrant returns (uint256) {
  if (shares == 0) {
    revert ZeroRedeem();
  }
  // Dedaub: an adversary can cause this to fail
  if (block.timestamp < lastDepositTime[owner] + COOLING_PERIOD) {
    IRebalancePool.RedeemingNotAvailableYet.selector.revertWith();
  }

	return super.redeem(shares, receiver, owner);
  }

This constraint opens the possibility for the following DoS attack. aTokens can only be created by the Protocol (via the Treasury) and as a consequence under normal operation only the Protocol should deposit them in the RebalancePool. However, RebalancePool::redeem is a permissionless function, hence a user can call it directly to obtain direct custody of their aTokens. Similarly, RebalancePool::deposit is permissionless, so any user with direct custody of aTokens can deposit them in the pool, with an arbitrary address as owner since deposit takes receiver as an argument. So the attack works as follows:

• A malicious user mints aTokens
• Then waits for COOLING_PERIOD to pass and receives custody of the tokens by calling RebalancePool::redeem.
• The victim sends a transaction to redeem their tokens
• The malicious user front-runs this transaction and calls RebalancePool::deposit for a tiny amount and the victim as receiver, resetting the victim’s COOLING_PERIOD.
• After the deposit is executed the victim’s redeem will fail due to the COOLING_PERIOD.

Note that the last step can be repeated, causing an indefinite DoS with minimal cost (since the deposited amount can be tiny).

To prevent this attack we recommend to make all RebalancePool operations permissioned (see also M1).

A design choice of Stable Jack v2, compared to v1, is that users should not have direct custody of aTokens. Instead, minted tokens are automatically deposited in the RebalancePool and users receive the corresponding shares in the pool.

However, RebalancePool::redeem is a permissionless function, so any user can call it to obtain direct custody of the tokens.

Note that this issue is related to the DoS possibility described in M1, but independently from the DoS (which could in principle be fixed in a permissionless way) the fact that users obtain direct custody of the tokens is by itself a violation of the protocol’s design.

The FullMath library is used to enable more complex operations that would overflow when broken up into individual operations—e.g., a multiplication followed by division, where the final result fits in 256 bits but the result of the multiplication would not. The library is copied from Uniswap code but a condition seems to have been weakened, permitting a silent overflow. Specifically, the condition in the code can be found below:

// Make sure the result is less than 2**256.
// Also prevents denominator == 0
if(denominator == 0) {
    revert("FullMath: division by zero");
}

In the original code, the condition was the stronger:

// Make sure the result is less than 2**256.
// Also prevents denominator == 0
require(denominator > prod1);

There are calculation inconsistencies in SuperToken. If this contract is used, it needs to be throughly tested.

Specific instances:

function deposit(
    	uint256 _baseIn,
    	uint24 _minYTTokenMinted,
    	uint24 _minVTTokenMinted,
    	address _paymentToken,
    	bytes32 _hookData
) external payable nonReentrant whenNotPaused override returns (uint256) {
    	DProtocol.MintParams memory VTparams = DProtocol.MintParams({
        	operationType: 0,  // Dedaub: magic constant
        	baseIn: _baseIn,
        	slippage: _minVTTokenMinted,
        	paymentToken: _paymentToken,
        	recipient: address(this),
        	hookData: _hookData
    	});

    	DProtocol.MintResult memory VTresult =
        IProtocol(protocol).mintToken{ value: msg.value }(groupKey, VTparams);

The slippage parameter is supplied by the caller of this function as an absolute amount, but on the callee, Protocol::mintToken, it is used as a slippage rate.

 _mint(msg.sender, totalMinted / _nav());
 emit Minted(msg.sender, groupKey.toId(), totalMinted / _nav(),
             YTresult.atMinted, totalVTMinted);

 return totalMinted / _nav();

The calls to _nav() after minting will return a different result than the call used during the minting, since the token’s total supply changes.

The TreasuryStateLibrary::fetchBaseTokenPrice gets the price of a wrong token (sAvax instead of the base token). Furthermore, it is supposed to return both a spot price and a TWAP, however it returns the price received by the oracle twice:

function fetchBaseTokenPrice(
  DTreasury.TreasuryState storage /*self*/,
  DTokenRegistry.GroupState memory group,
  Action _action
) internal view returns (uint256 _twap, uint256 _price) {
  bool isValid;
  address priceOracle = group.extended.priceOracle.toAddress();
  (isValid, _price) =
    IPriceOracle(priceOracle).getPrice(group.core.sAvax.toAddress());

  if (!isValid) revert ErrorInvalidTwapPrice();

  // Dedaub: no real TWAP, this is misleading
  return (_price, _price);
}

In the “fixes” commit, the above function is used to update state.baseTwapNav which is then used in FxStableMath::leverageRatio.

Note that if the underlying oracle in group.extended.priceOracle uses Chainlink Feeds (as PriceOracle does), then the lack of a TWAP is not a problem since the Chainlink prices are already volume weighted. However, there is no such stated requirement for group.extended.priceOracle and the fact that fetchBaseTokenPrice does not return a TWAP as claimed is misleading and can lead to concrete issues in future versions of the code.

The totalBaseTokens field of the TreasuryState keeps track of the amounts of base token that have explicitly (i.e., not implicitly through rebasing that has not yet been accounted for) moved to the Treasury. This is a crucial quantity, e.g., determining the harvestable yield amounts. However, the quantity is not consistently maintained when tokens are transferred. Instances include:

	groupState.core.baseToken.safeTransfer(_msgSender(), _amount);
	state.strategyUnderlying += _amount;
      // Dedaub: state.totalBaseTokens not decremented

uint256 usdcAmount =
  _swapTokens(swapRouter, uBaseToken, stablecoin, baseTokenAmount);
// Dedaub: baseToken exits the treasury, but
// treasuryStates[groupId].totalBaseTokens is not updated

groupState.core.baseToken.
  safeTransfer(groupState.extended.rebalancePool.toAddress(), yieldBaseTokens);
// Dedaub: baseToken exits the treasury, but
// groupStateToken.totalBaseTokens is not updated

(This function is called from harvestYield().)

Relatedly, there is a TreasuryStateLibrary::updateTotalBaseTokens internal function that is never called.

Treasury::harvestFees uses a base token amount as an aToken amount:

  aToken.mint(address(this), aTokenMintable);
  // Transfer aTokens to Rebalance Pool
  groupState.core.aToken.safeTransfer(address(rebalancePool), baseTokenAmount);

Function Treasury::_mintAndTransferYield, called from harvestYield, uses a base token amount as an aToken amount:

  aToken.mint(address(this), aTokenMintAmount);
  IERC20Upgradeable(address(aToken)).
    safeTransfer(groupState.extended.rebalancePool.toAddress(), yieldBaseTokens);

Function ProtocolUtils::prepareTokenMintForTreasury calls StabilityFeeOperations::getStabilityState, which expects a base token amount as its second argument. However, the caller supplies an sAvax (yield bearing token) amount:

StabilityFeeOperations.StabilityState memory stabilityState =
   StabilityFeeOperations.getStabilityState(context, sAvaxAmount);

The code below seems to be performing the wrong division:

uint256 precision =
  10 ** GroupSettings.wrap(groupState.groupSettings).getBaseTokenDecimals();
uint256 managed = getWrapppedValue(precision, totalBaseToken);
// Dedaub: merely divides by precision

Most protocol contracts (Protocol, Treasury, AToken, PriceOracle, TokenRegistry, several more; and in the fix commit also RebalancePool) are upgradeable and include field padding of the form:

uint256[50] private __gap; // Storage gap for upgradeability

This does not follow the usual upgradeability convention. The convention is to subtract from 50 the number of storage members already used and declare padding of this length, so that the next version can incrementally add fields and subtract the number of fields added from the __gap padding. The convention gives an easy way to check that the current padding is correct by inspecting how many storage members the contract currently has, instead of needing to know how many it had in the previous version. Starting from 50 would violate this property.

The RebalancePool::updateNAV function emits an event and performs no state changes other than updating a rate-limit mapping:

function updateNAV() public override {
  if (lastNavUpdate[_msgSender()] + UPDATE_PERIOD_RATE < block.number) {
	IRebalancePool.UpdatingNotAvailableYet.selector.revertWith();
  }
  lastNavUpdate[_msgSender()] = block.number;
  emit NAVUpdated(totalAssets(), totalSupply());
}

As a consequence it seems useless to have a per-user rate limit in such a function. Is the goal to avoid emitting too frequent events? If so the rate limit is not effective since we can trivially work around it using different senders.

There are view functions with msg.sender restrictions, for instance:

function getTreasuryState(
  GroupId groupId
) external view override validateGroup(groupId) onlyRole(DEFAULT_ADMIN_ROLE) returns (DTreasury.TreasuryState memory) {
  DTreasury.TreasuryState memory state = treasuryStates[groupId];

  return
	DTreasury.TreasuryState({
  	emaLeverageRatio: state.emaLeverageRatio,
  	inited: state.inited,
  	baseTokenPrice: state.baseTokenPrice,
  	totalBaseTokens: state.totalBaseTokens,
  	baseTokenCaps: state.baseTokenCaps,
  	strategyUnderlying: state.strategyUnderlying,
  	lastSettlementTimestamp: state.lastSettlementTimestamp
  })
}

Such restrictions are not necessarily problematic, but it is advisable to avoid them, not only to save gas, but also to avoid unintended effects. For instance, one should not rely on such call to make SomeOtherFunction() permissioned, cause in the future the call to the view function might be removed without the developer realizing that removing a view call might have permission implications.

There are two spots in the code where effect-free functions get called, expecting that they perform an update.

function updateEMALeverageRatio(DTreasury.TreasuryState storage self, F
                              xStableMath.SwapState memory _state) internal view {
	uint256 _ratio = _state.leverageRatio(_state.beta);
	self.emaLeverageRatio.saveValue(uint96(_ratio));
}

This function accepts a storage pointer yet does not update storage state: it calls a view function, saveValue, which takes a memory pointer (copied from the storage contents) and updates the memory contents. The updated data do not get copied back to storage.

context.params.slot1.setHookData(bytes32(0));

Slot1::setHookData is a pure function, performing no side-effects. The intent of the code was probably to call context.setHookData.

Function Treasury::currentBaseTokenPrice is using a cached value for the price. This is a dangerous practice, since the value requires continuous updating (by the admin).

function currentBaseTokenPrice(GroupId groupId) external view override validateGroup(groupId) returns (uint256) {
	return treasuryStates[groupId].baseTokenPrice;
}

Function RebalancePool::getNavPerShare tries to consult a price oracle (when available) to determine the price of an aToken. However, an invalid response is silently ignored.

(, price) = priceOracle.getPrice(address(aToken));  // Dedaub: isValid ret ignored
return price;

Function XToken::burn has no whenNotPaused modifier, in contrast to several other similar functions (e.g., mint, but also burn functions of other tokens).

The protocol admin can change virtually all important parameters of the system, including oracles, hook contracts, fees, etc., as well as receive amounts directly. Therefore, the admin must be fully trusted, both in terms of integrity and in terms of operational security (e.g., by using multi-signature accounts with properly decentralized keys, good processes for authorized actions, etc.).

CustomRevert::revertWith is used in most parts of the code to revert with a custom error. However, there are still parts of the code using the standard revert, for instance RebalancePool::redeem. It would be preferable to be consistent in the revert method.

Several parts of the code use a double safeApprove to overcome the limitation that safeApprove cannot be used to modify a non-zero approval. For instance:

function mintTokens(
  DOperationContext.OperationContext memory context,
  uint256 underlyingValue,
  address recipient
) internal returns (DProtocol.MintResult memory result) {
  ...

  // Approve treasury to spend baseTokens
  if (baseToken.allowance(address(this), address(treasury)) <
      stabilityState.boundedAmount) {
	baseToken.safeApprove(address(treasury), 0);
	baseToken.safeApprove(address(treasury), stabilityState.boundedAmount);
  }

  ...
}

Instead of the double call, safeIncreaseAllowance can be used to perform the same operation more cleanly and with less gas.

Compilation of test cases fails due to the following import

import { Constants } from "./constants/Constants.sol";

which fails in case-sensitive systems since the file is named constants.sol.

The code below performs an unnecessary calculation. Is there a logic error?

if (amount <= state.maxBaseInOrOut) {
  	feeAmount = (amount * feeRate) / BASIS_POINTS;
} else {
  	uint256 baseFeePortion = (state.maxBaseInOrOut * feeRate) / BASIS_POINTS;
  	uint256 remainingAmount = amount - state.maxBaseInOrOut;
  	uint256 remainingFee = (remainingAmount * feeRate) / BASIS_POINTS;
  	feeAmount = baseFeePortion + remainingFee;
     // Dedaub: This calculation computes the same feeAmount as in the "if" branch
}

We notice that this code has been removed in the fix commit.

The code fragments below perform unnecessary work (e.g., re-reading a storage variable, computing an already-computed quantity, etc.). The list is not comprehensive, only containing items that the auditors happened to notice.

revokeRole(TREASURY_ROLE, address(treasury));

emit UpdateTreasuryAddress(address(treasury), _newTreasury);
// Dedaub: reading treasury from storage again

bytes16 newDecayFactor = decayFactor;
// Dedaub: gets assigned again before read

if (feeModel == FeeModel.MANAGEMENT_FEE) {
…
  newDecayFactor = ABDKMathQuad.mul(decayFactor, decayMultiplier);
} else if (feeModel == FeeModel.FIXED_FUNDING_FEE) {
…
  newDecayFactor = ABDKMathQuad.mul(decayFactor, decayMultiplier);
} else if (feeModel == FeeModel.VARIABLE_FUNDING_FEE) {
…
  newDecayFactor = ABDKMathQuad.sub(decayFactor, feeIncrementQuad);
…
} else if (feeModel == FeeModel.NONE) {
  // No decay
  return decayFactor;
} else {
  IDXToken.InvalidFeeModel.selector.revertWith();
}

if (sender == address(0) || recipient == address(0))
  IDXToken.ZeroAddress.selector.revertWith();
…
if (sender != address(0) && recipient != address(0)) {
// Dedaub: already checked at the top

FeedInfo memory baseTokenFeedInfo = _info;
uint256 lstTokenPriceOracle = _fetchPrice(lstTokenFeedInfo);
uint256 baseTokenPrice = _fetchPrice(baseTokenFeedInfo);
// Dedaub: price is fetched again, it was already fetched at the caller
// of _isPriceValid

if (yieldFeePercentage > 0) {
  uint256 feeAmount = FullMath.mulDiv(yieldAmount, yieldFeePercentage, PRECISION);

  // Update state variables before any external call to prevent reentrancy
  lastTotalAssets = currentTotalAssets;
  updateNAV();

  if (feeAmount > 0) {
   	SafeERC20.safeTransfer(IERC20(asset()), feeCollector, feeAmount);
    	emit YieldFeeProcessed(yieldAmount, feeAmount);
  }
} else {
  // Only update lastTotalAssets and NAV if no fee to process
  lastTotalAssets = currentTotalAssets;
  updateNAV();
  // Dedaub: these two lines are performed on both branches of the "if". Why not
  // extract them before and eliminate the "else"?
}

function initializeGroup(GroupId groupId, DTreasury.GroupUpdateParams calldata params) external onlyRole(DEFAULT_ADMIN_ROLE) {
  …
  if (groupState.extended.treasury.toAddress() != address(this)) {
    _forceUpdateGroupCache(groupId);
    // Dedaub: ensures groupState.extended.treasury.toAddress() == this
  }

  groupState = cacheStorage.getGroupState(tokenRegistry, groupId);
  if (groupState.extended.treasury.toAddress() != address(this))
    revert InvalidGroupConfiguration(groupId);
  // Dedaub: unnecessary. It's ensured by earlier checks

  	let freeMemPtr := mload(0x40)  // Dedaub: unused

HookLibrary::beforeMint():227 and BeforeRedeem():311

    uint24 lpFeeOverride = uint24(lpFeeOverrideUint);  // Dedaub: already a uint24

DXToken::updateCoolingOffPeriod() and other similar-structure update functions after it:

function updateCoolingOffPeriod(uint256 newCoolingOffPeriod) external onlyRole(DEFAULT_ADMIN_ROLE) {
…
  if (newCoolingOffPeriod == coolingOffPeriod)
    IDXToken.ParameterUnchanged.selector.revertWith();
  emit UpdateCoolingOffPeriod(coolingOffPeriod, newCoolingOffPeriod);
  // Dedaub: re-read from storage

function updateTreasury(address _newTreasury) external onlyRole(DEFAULT_ADMIN_ROLE) {
   ...
   grantRole(TREASURY_ROLE, _newTreasury);
   // Dedaub: This checks again that the message sender has the admin role, so the
   // onlyRole(DEFAULT_ADMIN_ROLE) above is extraneous, or _grantRole could be
   // called
}

if (params.baseFee > MAX_GROUP_FEE) {
  // Dedaub: This check is implied by the combination of the last two
  BaseFeeExceedslimit.selector.revertWith();
}
...
// Max fee must be greater than or equal to base fee.
if (params.maxFee < params.baseFee) {
  MaxFeeIsLessThanBaseFee.selector.revertWith();
}
// Max fee should not exceed the maximum allowed group fee.
if (params.maxFee > MAX_GROUP_FEE) {
  MaxFeeExceedsLimit.selector.revertWith();
}

if (nativeCount > 1) {
  	collaterals.pop();
  	self.groupCollaterals[groupId][token] = false;
       // Dedaub: Unnecessary code. The transaction will revert anyway
	ITokenRegistry.MultipleNativeTokensNotAllowed.selector.revertWith();
}

There are additionally more optimization opportunities (e.g., re-reading storage already read inside a modifier in TokenRegistryState::get* functions) but these require more refactoring to be exposed, which may hinder code quality.

Some fields, functions, contracts, or types are unused or effectively unused (i.e., read and written to, but not used in any computation). Some of these may be used in the future, or in hooks.

• RebalancePool::_withdrawOtherERC20, addAdditionalToken
• contract NoDelegateCall
• library SlippageCheck
• DTreasury::GroupUpdateParams.rebalancePoolRatio, HarvesterRatio
• DTreasury::TreasuryState.rebalancePoolRatio, HarvesterRatio
• type BeforeOpsDelta and its library
• GroupFeeLibrary::_removeAllFlagsAndValidateBoundries
• BaseHook::selfOnly
• OperationContextLibrary::getProtocolFee, setProtocolFee, setSlot0, setSlot1, getStabilityFeeTriggerRatio
• Protocol::settleOwedFees
• functionality in contract SafeCallback
• WordCodec::insertAddress, decodeAddress, insertEnum, decodeEnum.

Hook functions _beforeMint, _afterBurn, in XToken, which are empty, cannot be overridden in sub-contracts, since they have not been declared virtual. They are, thus, completely extraneous.

  function _beforeMint(address _to, uint256 _amount) internal {
	// Custom logic before minting (if any)
  }

Similarly, afterMint and beforeBurn are not virtual (but are also not empty).

Parts of the code use explicitly msg.sender, thus will not function correctly with possible future extensions that employ meta-transactions or other msg.sender abstraction. Not all uses of msg.sender should be _msgSender(), since several functions are called by other contracts inside the protocol (e.g., msg.sender is the Protocol or the Treasury). If _msgSender() returns something other than msg.sender in the future, the codebase (and test suite) will need to be revisited throughout.

There are a few spots where the name of an entity is not consistent with other code elements. We group the ones we observed below.

uint256 private constant MIN_MINTING_BURNING_AMOUNT = 1e6; // 0.001 tokens
// Dedaub: comment seems off?

if (baseIn <= acceptableCollaterals[i].minAmount ||
  baseIn >= acceptableCollaterals[i].maxAmount)
{  // Dedaub: min and max not inclusive?
        	IProtocol.InvalidMinMaxCollateralAmount.selector.revertWith();
}

/**
 * @dev Maximum allowed group fee (10,000 BPS). 50% expressed in basis points.
 */
uint24 public constant MAX_GROUP_FEE = 50_000;
// Dedaub: not 50% in basis points, but 500%

bool isAboveTrigger = collateralRatio <= uint256(stabilityFeeTriggerRatio);
// Dedaub: inverted name, correct logic

if (setup.meta.stabilityRatio > MAX_STABILITY_RATIO ||
    setup.meta.stabilityRatio < setup.meta.stabilityFeeTriggerRatio) {
  	ITokenRegistry.StabilityRatioTooLarge.selector.revertWith();
    // Dedaub: not quite the right revert message (one of the conditions is
    // "too large", the other is "smaller than ...")
}

NativeCurrencyTransferFromNotAllowed.selector.revertWith();
// Dedaub: not quite right revert message, we are in transfer, not transferFrom

 * @param convertAmount The address of the swap router used for token conversion.
 * @param convertAddress The address of the swap router used for token conversion.
 * @param swapRouter The address of the token to swap from (USDC).
// Dedaub: copy-paste mistakes in comments

Furthermore, the convention that identifiers in capital letters denote constants is not consistently maintained (e.g., RebalancePool::COOLING_PERIOD).

The code is compiled with Solidity 0.8.28. Version 0.8.28 has no known bugs at this time.