Part 2

Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Submission Details
Severity: low
Valid

Low issues collection

L-01 Curve adapter is not working on Arbitrum

Summary

CurveAdapter is using outdated Curve Registry Exchange Contract, which was never deployed to Arbitrum.

Vulnerability Details

Curve adapter is using exchange_with_best_rate function of Curve Registry Exchange Contract. According to curve doc, this contract was deployed to Mainnet on Dec 2022, and got outdated.

The contract was never deployed to Arbitrum.

And even on mainnet, it has very few available pools (only supports USDC <-> USDT, WETH <-> USDT, WBTC <-> USDT, considering all in-scope tokens)

Impact

  • Any operation involving CurveAdapter will revert

Tools Used

Manual review

Recommendations

Use CurveRouterNG, which also got deployed to Arbitrum

L-02 Not fully ERC-7201 compatible

Summary

Some market making leaves are not fully ERC-7201 compatible.

Vulnerability Details

The following is ERC-7201 formula

keccak256(abi.encode(uint256(keccak256(bytes(id))) - 1)) & ~bytes32(uint256(0xff))

However, the following storage locations don't apply & ~bytes32(uint256(0xff)):

  • ASSET_SWAP_STRATEGY_LOCATION

  • COLLATERAL_LOCATION

  • CREDIT_DELEGATION_LOCATION

  • DEX_SWAP_STRATEGY_LOCATION

  • MARKET_FEE_LOCATION

  • MARKET_LOCATION

  • MARKET_MAKING_ENGINE_CONFIGURATION_LOCATION

  • SWAP_LOCATION

  • USD_TOKEN_SWAP_CONFIG_LOCATION

  • VAULT_LOCATION

  • WITHDRAWAL_REQUEST_LOCATION

Moreover, all storages don't apply NatSpec tag annotation

Impact

  • Incompatibility with ERC-7201, unlike what's stated in the doc

Tools Used

Manual Review

Recommendations

  • Apply 0xff negation mask

  • Apply NatSpec tag annotation to storage data

L-03 Unable to set autoDeleverageStartThreshold and autoDeleverageExponentZ to 0

Summary

MarketMakingEngineConfigurationBranch.configureMarket prevents setting market's autoDeleverageStartThreshold and autoDeleverageExponentZ to 0, even though 0 is totally valid value for those parameters

Vulnerability Details

According to Market.getAutoDeleverageFactor, one can derive the following formula:

where

  • is autoDeleverageStartThreshold

  • is autoDeleverageEndThreshold

  • is autoDeleverageExponentZ

autoDeleverageFactor

For the above formula, 0 is a totally valid value for autoDeleverageStartThreshold and autoDeleverageExponentZ. For example:

  • If you want to set autoDeleverageFactorto 1 for all possible scenarios, you can set to autoDelverageExponentZto 1

  • If you want always set non-1 autoDeleverageFactorfor all possible scenarios, you should set autoDeleverageStartThresholdto 0

However, MarketMakingEngineConfigurationBranch.configureMarket prevents such setup:

// revert if autoDeleverageStartThreshold is set to zero
if (autoDeleverageStartThreshold == 0) revert Errors.ZeroInput("autoDeleverageStartThreshold");
// revert if autoDeleverageEndThreshold is set to zero
if (autoDeleverageEndThreshold == 0) revert Errors.ZeroInput("autoDeleverageEndThreshold");
// revert if autoDeleverageExpoentZ is set to zero
if (autoDeleverageExponentZ == 0) revert Errors.ZeroInput("autoDeleverageExponentZ");

Impacts

One will have to set autoDeleverageStartThreshold and autoDeleverageExponentZ to dust values (1e-18) to achieve the same behavior.
However this approach will waste gas and bring precision error.

Recommendation

Remove ZeroInput checks for autoDeleverageStartThreshold and autoDeleverageExponentZ

L-04 Different usd token engine might be used between initiateSwap and fulfillSwap

Summary

In StabilityBranch, user can initiate a request to swap usd token to vault asset. Later, system keeper can fulfill the swap request. However, usd token used when initiating a swap is a usd token engine of corresponding vault, while usd token usded in fulfill swap is a parameter of the function. This inconsistency might bring swap request failure. Refund request can also fail due to similar reason.

Vulnerability Details

When user initiates a swap, current vault engine's usd token is transferred from user to market making engine:

// transfer USD: user => address(this) - burned in fulfillSwap
ctx.usdTokenOfEngine = IERC20(configuration.usdTokenOfEngine[currentVault.engine]);
ctx.usdTokenOfEngine.safeTransferFrom(msg.sender, address(this), amountsIn[i]);

However, when the swap request is fulfilled, a usd token corresponding to engine parameter of the system keeper is burned:

function fulfillSwap(
address user,
uint128 requestId,
bytes calldata priceData,
address engine
)
external
onlyRegisteredSystemKeepers
{
...
ctx.usdToken = UsdToken(marketMakingEngineConfiguration.usdTokenOfEngine[engine]);
...
ctx.usdToken.burn(ctx.amountIn);
...

Impact

When multiple usd tokens are used, different usd tokens might be used during initiateSwap and fulfillSwap.
In this case, fulfillSwap might revert with insufficient balance error.
refundSwap can also fail due to similar reason i.e. refundSwap uses engine parameter instead of vault's engine.

Tools Used

Manual Review

Recommendation

Use vault.engine instead of function parameter engine in StabilityBranch.fulfillSwap and StabilityBranch.refundSwap

L-05 Vault.recalculateVaultsCreditCapacity does not check vaults and markets live status

Summary

Vault.recalculateVaultsCreditCapacity is an important function to update states of connected vaults and markets. However, it does not take live status into consideration. It can lead to invalid accounting of vaults and markets.

Vulnerability Details

Vault.recalculateVaultsCreditCapacity updates connected vaults, markets and credit delegation's state vars such as:

  • market's realized debt and realized debt per vault share

  • market's unrealized debt and unrealized debt per vault share

  • usdc credit per vault share

  • weth reward distribution per vault

  • update credit delegation's last distributed values

However, the function does not check wether vaults and markets are live. For example, the following code snippet loads vault even if it's not live

uint128 vaultId = vaultsIds[i].toUint128();
// load the vault storage pointer
Data storage self = load(vaultId);

Also, the following code snippet does not check market's live status:

// loads the market storage pointer
Market.Data storage market = Market.load(rehydratedConnectedMarketsIdsCache[i]);

Impact

Let's consider a scenario when a vault needs to be delisted from the protocol. One needs to do the following:

  • Call MarketMakingEngineConfigurationBranch.updateVaultConfigurationwith isLive = false

  • Get all market IDs that were previuosly connected to the vault (unsure how to do that onchain, maybe someone needs to manage an excel sheet or dig into EVM storage)

  • For all the above market IDs, call MarketMakingEngineConfigurationBranch.connectVaultsAndMarkets with vaultIds excluding the given vault's ID

This approach is tedious and error-prone.

If a market is overlooked in the above approach and still uses unlisted vault as connected one, the above vulnerability will lead to incorrect accounting of vault's total credit capacity and market's total credit capacity.

Furthermore, since CreditDelegationBranch.updateVaultCreditCapacity(uint128 vaultId) is external, anyone can call this function and update paused vault's state.

Tools Used

Manual Review

Recommendations

  • Use loadLiveinstead of load, or

  • Introduce a safe approach to unlist a vault/market on-chain programmatically

L-06 When initiating usd token swap, expectedAssetOut does not consider protocol fee

Summary

When initiating usd token swap, expectedAssetOut does not consider protocol fee. However, when fulfilling swap, assetOutAmount is deducted by swap fee and base fee. This inconsistency can bring unwanted revert due to slippage check.

Vulnerability Details

Users can request swapping usd token to vault's underlying asset. When a swap request is initated, the protocol checks if expected asset amount out is more than requested minimum amount out.

ctx.expectedAssetOut = // @audit doesn't consider fee
getAmountOfAssetOut(vaultIds[i], ud60x18(amountsIn[i]), ctx.collateralPriceX18).intoUint256();
// revert if the slippage wouldn't pass or the expected output was 0
if (ctx.expectedAssetOut == 0) revert Errors.ZeroOutputTokens();
if (ctx.expectedAssetOut < minAmountsOut[i]) {
revert Errors.SlippageCheckFailed(minAmountsOut[i], ctx.expectedAssetOut);
}

However, when fulfilling swap, amountOut will be less than expectedAssetOut due to fee deduction:

ctx.amountOutBeforeFeesX18 = getAmountOfAssetOut(ctx.vaultId, ud60x18(ctx.amountIn), ctx.priceX18);
// gets the base fee and swap fee for the given amount out before fees
(ctx.baseFeeX18, ctx.swapFeeX18) = getFeesForAssetsAmountOut(ctx.amountOutBeforeFeesX18, ctx.priceX18);
// cache the collateral asset address
ctx.asset = vault.collateral.asset;
// load the collateral configuration storage pointer
Collateral.Data storage collateral = Collateral.load(ctx.asset);
// subtract the fees and convert the UD60x18 value to the collateral's decimals value
ctx.amountOut =
collateral.convertUd60x18ToTokenAmount(ctx.amountOutBeforeFeesX18.sub(ctx.baseFeeX18.add(ctx.swapFeeX18)));
// slippage check
ctx.minAmountOut = request.minAmountOut;
if (ctx.amountOut < ctx.minAmountOut) {
revert Errors.SlippageCheckFailed(ctx.minAmountOut, ctx.amountOut);
}

This inconsistency might bring unwanted revert on fulfilling swap due to slippage check failure.

Impact

Users will lose fund because when swap request fails, they have to pay protocol fee.

Recommendation

Expected amount out should consider fees on initiateSwap.

Updates

Lead Judging Commences

inallhonesty Lead Judge
4 months ago
inallhonesty Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

ERC7201 implemented incorrectly

Missing basefee and settlement fee deduction before slippage check in the Stablitybranch contract.

CurveAdapter does not enforce swap execution time limits like other adapters do

Appeal created

inallhonesty Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

ERC7201 implemented incorrectly

Missing basefee and settlement fee deduction before slippage check in the Stablitybranch contract.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.