Stable Jack audit report

ABSTRACT

Dedaub was commissioned to perform a security audit of the Stable Jack v.2 protocol. Stable Jack is a DeFi protocol allowing investors to place a leveraged bet on a token or to receive higher yield, with the two kinds of bets complementing each other. The protocol has gone over a significant reengineering for v.2. The audit had several significant findings that were already addressed during the initial audit course, since the code had been actively evolving.

BACKGROUND

Stable Jack is a DeFi protocol that accepts deposits in a yield-bearing token and balances investors who want to receive higher yield against investors who are willing to forego their yield in favor of higher leverage, i.e., a higher upside in case the underlying token grows in value. Upon depositing their yield-bearing token, an investor can opt to receive one of two investment tokens: a yield token (YT) or a volatility token (VT or “xToken”). Yield tokens allow the minting of an associated stablecoin, the aToken for the protocol. When yield is harvested, more aToken is minted, and is claimable by the yield token holders. Conversely, volatility investors do not receive yield. However, if the collateral appreciates, VT holders reap the full benefit of appreciation, both from their own investment and from that of yield token holders. The protocol documentation explains the high-level motivation and business case in detail.

The audit is over v.2 of the protocol, which boasts several features, the main of which is support for multiple collaterals.

SETTING & CAVEATS

This audit report mainly covers the contracts of the at-the-time private repository stable-jack/smart-contracts-v2 of the Protocol Stable Jack at commit d031e8d2be7f26f9bec0c08e2538cdd91800e836 (the “base commit”). The review of most fixes is relative to commit 660c0773e7de160c2cec206384835290ba6337c2 (the “fixes commit”), also with final inspection (for issues H1, L2, L3, L7) at commit 479f51586c94a062fc277b62944e8659c10f252d. The latter commits were examined closely but not necessarily exhaustively, i.e., they were the subject of a partial “delta” audit relative to the initial functionality and not a full re-audit. Thus, the development team should be particularly vigilant in testing functionality that was developed after the base commit and that is not a fix of issues identified in this report.

2 auditors worked on the codebase for 25 days on the following contracts:

src//
├── abstracts/
│   ├── BaseHook.sol
│   └── NoDelegateCall.sol
├── AToken.sol
├── declarations/
│   ├── DDXToken.sol
│   ├── DOperationContext.sol
│   ├── DProtocol.sol
│   ├── DTokenRegistry.sol
│   ├── DTreasury.sol
│   └── DUXToken.sol
├── DeployerAndUpgradeManager.sol
├── DXToken.sol
├── ImmutableState.sol
└── interfaces/
    ├── libs/
    │   ├── CacheLibrary.sol
    │   ├── CustomRevert.sol
    │   ├── FeeManagementLibrary.sol
    │   ├── GroupFeeLibrary.sol
    │   ├── HookLibrary.sol
    │   ├── math/
    │   │   ├── ExponentialMovingAverageV8.sol
    │   │   ├── FullMath.sol
    │   │   ├── FxStableMath.sol
    │   │   └── LogExpMathV8.sol
    │   ├── OperationContextLibrary.sol
    │   ├── ProtocolConfigState.sol
    │   ├── ProtocolUtils.sol
    │   ├── SafeCallback.sol
    │   ├── StabilityFeeOperations.sol
    │   ├── TimeManagementLibrary.sol
    │   ├── TokenRegistryState.sol
    │   ├── TreasuryStateLibrary.sol
    │   └── WordCodec.sol
    ├── PriceOracle.sol
    ├── PriceOracle.sol_production
    ├── Protocol.sol
    ├── rateProviders/
    │   └── WSAVAX.sol
    ├── RebalancePool.sol
    ├── SuperToken.sol
    ├── TokenRegistry.sol
    ├── Treasury.sol
    ├── types/
    │   ├── Address.sol
    │   ├── CommonTypes.sol
    │   ├── Currency.sol
    │   ├── GroupId.sol
    │   ├── GroupKey.sol
    │   ├── GroupStateHelper.sol
    │   └── Slot0.sol
    ├── wDXToken.sol
    ├── WToken.sol
    └── XToken.sol

The audit’s main target is security threats, i.e., what the community understanding would likely call “hacking”, rather than the regular use of the protocol. Functional correctness (i.e. issues in “regular use”) is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.

PROTOCOL-LEVEL CONSIDERATIONS

The protocol implementation exhibits significant technical sophistication. This is a large, ambitious implementation effort. However, the starting point of the audit was a codebase still in development, with clear signs of immaturity. The code and tests have been evolving throughout the course of the audit, resulting in much higher confidence of correctness. Notably, most issues identified in the audit are issues of functional correctness and not security. Testing is the most effective way to detect these issues. Consequently, in this report we sometimes downplay issues—e.g., a simple calculation oversight, detected and fixed during the audit period, may be rated Medium-severity instead of High (as would have been the case if the protocol were reviewed in “ready to be deployed” state).

In addition, there are specific considerations that need to be pointed out, at the global level. These are not vulnerabilities but items that require caution.

Consistency of configuration/deployment is crucial

Protocol Level Consideration | Status: INFO

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.)

The protocol should strongly avoid getting under-collateralized

Protocol Level Consideration | Status: INFO

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.

VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.

CRITICAL SEVERITY

Minimum expected price from a DEX swap is not derived from an off-chain source

Critical | Status: RESOLVED

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:

Treasury::_swapTokens():730

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.

HIGH SEVERITY

Wrong calculation when harvesting fees

High | Status: RESOLVED

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.

DXToken:195

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.

MEDIUM SEVERITY

DoS in RebalancePool::redeem

Medium | Status: RESOLVED

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.

RebalancePool::redeem():199

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).

Tokens can be removed from the RebalancePool

Medium | Status: RESOLVED

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.

(Not fully-determined severity) The FullMath library is missing an overflow check

Medium | Status: RESOLVED

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:

FullMath::mulDiv():28

// 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);

Calculation inconsistencies in SuperToken

Medium | Status: RESOLVED

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

Specific instances:

SuperToken:97

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.

SuperToken::deposit():139

 _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.

TreasuryStateLibrary::fetchBaseTokenPrice uses wrong token, does not return a TWAP

Medium | Status: LARGELY RESOLVED

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:

TreasuryStateLibrary::fetchBaseTokenPrice():137

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.

TreasuryState.totalBaseTokens not consistently updated

Medium | Status: RESOLVED

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:

Treasury::transferToStrategy():497

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

Treasury::rebalanceUp():648

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

Treasury::_mintAndTransferYield():793

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.

Wrong token amount used in harvestFees

Medium | Status: RESOLVED

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

Treasury::harvestFees():563

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

Wrong token amount used in harvestYield

Medium | Status: RESOLVED

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

Treasury::_mintAndTransferYield():795

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

Wrong token amount used in ProtocolUtils::prepareTokenMintForTreasury

Medium | Status: RESOLVED

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:

ProtocolUtils::prepareTokenMintForTreasury():91

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

Wrong calculation in Treasury::harvestable

Medium | Status: RESOLVED

The code below seems to be performing the wrong division:

Treasury::harvestable():291

uint256 precision =
  10 ** GroupSettings.wrap(groupState.groupSettings).getBaseTokenDecimals();

uint256 managed = getWrapppedValue(precision, totalBaseToken);
// Dedaub: merely divides by precision

LOW SEVERITY

__gap convention for upgradeability is error-prone

Low | Status: OPEN

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.

Strange per-sender rate limit in RebalancePool::updateNAV

Low | Status: RESOLVED

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

RebalancePool::updateNAV():280

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.

Permissioned view functions

Low | Status: RESOLVED

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

Treasury::getTreasuryState():959

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.

(Two) No-op computations that should instead be updates

Low | Status: RESOLVED

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

ProtocolUtils:245

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.

OperationContextLibrary::CreateContext():88

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.

baseTokenPrice is being cached, requires continuous updating

Low | Status: RESOLVED

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).

Treasury:239

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

Validity of oracle prices ignored (RebalancePool)

Low | Status: RESOLVED

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.

RebalancePool::getNavPerShare():302

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

XToken::burn can be called when the token is paused

Low | Status: RESOLVED

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

CENTRALIZATION ISSUES

It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocol’s owner. Even if the owner is reputable, users are more likely to engage with a protocol that guarantees no catastrophic failure even in the case the owner gets hacked/compromised. We list issues of this kind below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-profile, high-value protocols have significant centralization threats.)

The protocol admin has full authority and responsibility

Centralization | Status: ACKNOWLEDGED

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.).

OTHER / ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project (thus can be safely ignored), but we recommend considering them.

No consistent revert method

Advisory | Status: INFO

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.

safeIncreaseAllowance can be used to avoid the double safeApprove

Advisory | Status: INFO

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:

ProtocolUtils::mintTokens():213

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.

Respect filename case in imports

Advisory | Status: INFO

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.

Calculation seems wrong/redundant

Advisory | Status: INFO

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

StabilityFeeOperations::calculateFee():80

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.

Some wasteful computations

Advisory | Status: INFO

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.

AToken::updateTreasury():134

revokeRole(TREASURY_ROLE, address(treasury));

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

DXToken::_getUpdatedDecayFactor():208

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();
}

DXToken::_transfer():391

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

PriceOracle::_isPriceValid():180

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

RebalancePool::_processYieldFee():258

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"?
}

Treasury:159

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

HookLibrary::callHook():163

  	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

XToken:169

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
}

GroupFeeLibrary::_validateFeeParams():183

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();
}

TokenRegistryState::_addCollateral():488

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.

Unused elements

Advisory | Status: INFO

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.

(Empty) hook functions in XToken cannot be overridden

Advisory | Status: INFO

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.

XToken:202

  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).

msg.sender and Context::_msgSender() are both used, sometimes inconsistently

Advisory | Status: INFO

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.

Naming issues and inconsistencies in identifiers, revert messages, comments

Advisory | Status: INFO

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.

DXToken:41

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

Protocol:_isPaymentTokenSupported():349

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

GroupFeeLibrary:68

/**
- @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%

OperationContextLibrary::_calculateContextFee():158

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

TokenRegistryState::_validateGroupSetup():231

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 ...")
}

Currency::safeTransfer():113

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

Treasury:658

- @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).

Compiler bugs

Advisory | Status: INFO

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

DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.

ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub’s auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

SUMMARY

ABSTRACT

BACKGROUND

SETTING & CAVEATS

PROTOCOL-LEVEL CONSIDERATIONS

Consistency of configuration/deployment is crucial

The protocol should strongly avoid getting under-collateralized

VULNERABILITIES & FUNCTIONAL ISSUES

CRITICAL SEVERITY

Minimum expected price from a DEX swap is not derived from an off-chain source

HIGH SEVERITY

Wrong calculation when harvesting fees

MEDIUM SEVERITY

DoS in RebalancePool::redeem

Tokens can be removed from the RebalancePool

(Not fully-determined severity) The FullMath library is missing an overflow check

Calculation inconsistencies in SuperToken

TreasuryStateLibrary::fetchBaseTokenPrice uses wrong token, does not return a TWAP

TreasuryState.totalBaseTokens not consistently updated

Wrong token amount used in harvestFees

Wrong token amount used in harvestYield

Wrong token amount used in ProtocolUtils::prepareTokenMintForTreasury

Wrong calculation in Treasury::harvestable

LOW SEVERITY

__gap convention for upgradeability is error-prone

Strange per-sender rate limit in RebalancePool::updateNAV

Permissioned view functions

(Two) No-op computations that should instead be updates

baseTokenPrice is being cached, requires continuous updating

Validity of oracle prices ignored (RebalancePool)

XToken::burn can be called when the token is paused

CENTRALIZATION ISSUES

The protocol admin has full authority and responsibility

OTHER / ADVISORY ISSUES

No consistent revert method

safeIncreaseAllowance can be used to avoid the double safeApprove

Respect filename case in imports

Calculation seems wrong/redundant

Some wasteful computations

Unused elements

(Empty) hook functions in XToken cannot be overridden

msg.sender and Context::_msgSender() are both used, sometimes inconsistently

Naming issues and inconsistencies in identifiers, revert messages, comments

Compiler bugs

DISCLAIMER

ABOUT DEDAUB